-
Notifications
You must be signed in to change notification settings - Fork 388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite the frontend #1856
base: main
Are you sure you want to change the base?
Rewrite the frontend #1856
Conversation
I've added functionality for the top banner here, and poked around to make sure that the existing banner can display well. It needs to be redone to use bootstrap 5 utility classes, but here it is:
This works well! |
Let's no longer special case Google Analytics
This is handled by npm now
Have asked @oliverroick to do a review here since his React eyes are much, much sharper than mine. |
UI related stuff like this goes into spec.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, this looks pretty solid. I found two occasions where you probably don't need a useEffect
hook and the launch logic could be improved, I think, although I don't have the full context to make a better suggestion.
useEffect(() => { | ||
// Start launching after the DOM has fully loaded | ||
setTimeout(() => setIsLaunching(true), 1); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit flaky. Is there a specific element you're waiting for in the DOM tree? If so how is it created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was trying to trigger this useEffect (https://github.com/jupyterhub/binderhub/pull/1856/files#diff-17ed39387f14113cf305cbfd7cc95014fc635d5988acbe542676dd87695c967cR185) and couldn't figure out how else to do it :( In general I think I was trying to trigger 'actions' in one component based on events in a different component, and without using reducers i think i'm hackily using these 'is' booleans to do so
binderhub/main.py
Outdated
class MainHandler(BaseHandler): | ||
"""Main handler for requests""" | ||
def initialize(self): | ||
self.opengraph_title = "The Binder Project" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this configurable in future, since not all BinderHubs are run by us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%. Do you think that should be done as part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that why I said in future 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manics i made it configurable :)
export const PAGE_CONFIG = window.pageConfig; | ||
|
||
/** | ||
* @typedef {object} RepoConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can verify type annotations if they're provided, to force people to keep them in sync with future code changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@manics soooo you can run npx tsc --noEmit --checkJs
locally to have typescript validate these (it'll read JS, doesn't require typescript). However, the validation doesn't actually pass, mostly because our spec isn't fully complete. Given this PR has already taken so long, I want to not block this PR on that. Does that seem ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it's not a blocker. You're the only one who knows about it though, so can you make a note of it somewhere, either an issue, in the docs, or somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new interface is a lot narrower than the existing one, and the form background is lighter which I find makes it difficult to distinguish the input fields from the text labels, especially since the input fields have placeholder text that looks very similar to the label text. Do you think you could change the styling to make the input fields more obvious? |
Co-authored-by: Simon Li <[email protected]>
Co-authored-by: Simon Li <[email protected]>
Co-authored-by: Simon Li <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Simon Li <[email protected]>
nvm, i was wrong |
@manics this is how it looks now. what do you think? While the form controls didn't flag in the WCAG Contrast Checker some other things did. I'll poke at those. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot clearer!
Co-authored-by: Simon Li <[email protected]>
After working on jupyterhub/repo2docker#1393, I was able to get a better handle on what we actually support with hydroshare in binderhub and clarify that (only ids, not URLs). No change from before this PR, just clearer. |
Co-authored-by: Simon Li <[email protected]>
Co-authored-by: Simon Li <[email protected]>
Co-authored-by: Simon Li <[email protected]>
This was the first line I wrote when I started writing the existing frontend, and of course that line is still there.
This PR cleans up and rewrites the entire frontend, to 'start using a framework'. There's no functional change to the UI itself, so it should be treated as a pure upgrade / refactor.
Demo
Main page
Screen.Recording.2024-05-15.at.6.03.12.PM.mov
Loading page
Screen.Recording.2024-05-23.at.8.21.36.AM.mov
Functional Status
The following functional pieces need to be completed:
Technology changes
_config
endpoint to a refactored/api/repoproviders
. This is with an eye on allowing us to implement Awesome bar/landing page redesign #844 eventually, as well as being able to implement the correct frontend bits in other users of the binderhub API (like https://github.com/yuvipanda/jupyterhub-fancy-profiles)extra_footer_scripts
can continue to be used - that's what we use for matomo.Functionality changes
nbgitpuller
, the link generator now prefers outputting onlyurlpath
- both when the user enters a file to open or a URL to open. This prevents the issue we had when we tried to change the default app that was going to open from classic notebook to lab, and broke a lot of people's stuff. By only outputtingurlpath
, URLs will always have this information encoded in them. The olderfilepath
andlabpath
are still accepted as input, because Cool URIs don't breakMaintenance changes
One primary goal here is to make the frontend safer to change, so it's less fragile and brittle.
jsdoc
inline documentation for everythingbinderhub-client
package to make sure it only contains api-client related functionality - all UI stuff should be in a separate package.js/
and thebinderhub/static/js
sources of JS files. Pick up best practices from other Jupyterish projects for what to do here.Timeline
My hope is to slowly but consistently work on this, and get it fully complete before end of June. I have also asked for some frontend review help from @batpad (either directly or via someone else), as he has significant experience in this kinda frontend work (even though he is less experienced in the JupyterHub community itself).
Fixes #774