-
Notifications
You must be signed in to change notification settings - Fork 323
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
Clean up git menu #699
Clean up git menu #699
Conversation
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.
Thank you for this @mlucool.
Three comments:
- Could you use
isEnabled
rather thanisVisible
? In JupyterLab it is preferred to not hide element but to disable them when they are not available. - Could you rephrase
Init
andClone
to be more verbose for inexperienced users? Taking the same wording than for the buttons (jupyterlab-git/src/components/GitPanel.tsx
Line 332 in 103ea8d
Initialize a Repository - Could you add the commands:
Add remote repository
Push to remote
Pull to remote
For the two latest, this means creating new commands. But the code is available:
jupyterlab-git/src/components/Toolbar.tsx
Line 306 in 103ea8d
private _onPullClick = (): void => {
Note: change the linked code above to call the new commands rather than the code directly - to ensure behavior homogeneity
Happy to add more commands/expand wording. Is there written documentation on the visible vs. enable choice? |
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 like it. @mlucool Nice cleanup and improvement
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.
Thanks a lot @mlucool
This is really nice indeed. I'm working on a fix for the unit test. Then it will be good to go.
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.
Unit tests fixed!
@meeseeksdev backport to 0.11.x |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon! If these instruction are inaccurate, feel free to suggest an improvement. |
Clean up git menu
Fixes #456
Not in a git repo:
In a git repo: