-
Notifications
You must be signed in to change notification settings - Fork 313
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
Git ignore UI #705
Git ignore UI #705
Conversation
Looks great! Nitpick: I think there is an extra blank line between entries. Most projects gitignore uses whitespace for grouping. In this case, I would consider them all one group. |
Thx @mlucool yeah I don't like the extra blank line. It is there to avoid appending on the same last line if no I will wait a bit any feedback and will implement those for final review. |
Thanks a lot @echarles. This looks really nice. And if indeed you could improve the code to introduce the line return only if needed. This will be awesome. |
Do you think ignore extension is common? If so, to prevent user error, maybe we should disable ignore extension at on ipynb and maybe py files? Also maybe we should say |
@mlucool Ignoring all files having a given extension is supported by the GitHub desktop app, which is discussed in the linked issue and I believe the inspiration for the behavior here. |
Yes, I have quickly added that based on @ianhi input in #683 (comment). I can remove it if we don't want that. I am trying to get a better message for the right-click menu with something like the following which does not work - any idea to get a more dynamic right click menu with commands.addCommand(CommandIDs.gitIgnore, {
label: 'Ignore',
caption: `Ignore ${this.state.selectedFile.to}`, If we keep the Waiting on more input/feedback here. |
What about that is not working? Also https://jupyterlab.github.io/jupyterlab/modules/_coreutils_src_index_.pathext.html#extname may be a helpful thing to use. Two tweaks I might make to phrasing in order to make it more obvious what is happening to users less experienced with Git:
Overall I'm a big fan of where this going! |
Although, rather than speculate as to what would be less confusing to users it may be better to ask one. I think @sjhaque14 is a good representative of users as she is a competent python user who uses jupyterlab for her work, but is fairly new to using Git. Sabina I'm curious what you would expect the result of the proposed menu commands to be? Also, would you expect the change to the |
Thx @ianhi for the help and inputs. In the meantime, I have done a few more experiments to bring the file/extension name in the context menu (when the user right-clicks a file). It turns out the current Lumino Widget constructs is such that new commands needs to be created with the correct label... which we do not want (commands are meant to remain controlled in number and unique in intention). We still have the option to have static and user-relevant naming as suggested which would be good enough. I will tag this ready for review in a few days. |
Thanks @ianhi for the mention. From a first glance, it is clear to me how to use the basic function – you go to the "Git" menu and select "Open .gitignore" to view the |
@echarles, I think the solution is to set the label and/or caption using a function caption: () => `Ignore ${this.state.selectedFile.to}` |
I have tried that and it also gives null pointer (without the function it fails directly without displaying the list, with the function it fails when you right click) I have updated the label and the |
@echarles Why the exceptions for Given that (a) Jupyter can be used for languages other than Python and thus scenarios in which a user may want to ignore all Python files is possible and (b) I may, e.g., be working on a JLab extension but never want notebooks added to the repo (in fact, excluding temporary demo notebooks is something I do relatively frequently when developing extensions), I don't see the need to make those two file extensions exceptions. |
Have taken that requirement from @mlucool on #705 (comment) |
@echarles I found the problem for the label: --- a/src/components/FileItem.tsx
+++ b/src/components/FileItem.tsx
@@ -33,7 +33,7 @@ export interface IFileItemProps {
model: GitExtension;
onDoubleClick: () => void;
selected?: boolean;
- selectFile?: (file: Git.IStatusFile | null) => void;
+ selectFile?: (file: Git.IStatusFile | null) => Promise<void>;
}
export interface IGitMarkBoxProps {
@@ -87,9 +87,9 @@ export class FileItem extends React.Component<IFileItemProps> {
}
onContextMenu={
this.props.contextMenu &&
- (event => {
+ (async event => {
if (this.props.selectFile) {
- this.props.selectFile(this.props.file);
+ await this.props.selectFile(this.props.file);
}
this.props.contextMenu(event);
})
diff --git a/src/components/FileList.tsx b/src/components/FileList.tsx
index e8a8592..1c7dcbd 100644
--- a/src/components/FileList.tsx
+++ b/src/components/FileList.tsx
@@ -2,6 +2,7 @@ import { Dialog, showDialog, showErrorMessage } from '@jupyterlab/apputils';
import { PathExt } from '@jupyterlab/coreutils';
import { IRenderMimeRegistry } from '@jupyterlab/rendermime';
import { ISettingRegistry } from '@jupyterlab/settingregistry';
+import { PromiseDelegate } from '@lumino/coreutils';
import { Menu } from '@lumino/widgets';
import * as React from 'react';
import { GitExtension } from '../model';
@@ -168,7 +169,10 @@ export class FileList extends React.Component<IFileListProps, IFileListState> {
if (!commands.hasCommand(CommandIDs.gitIgnoreExtension)) {
commands.addCommand(CommandIDs.gitIgnoreExtension, {
- label: 'Ignore this file extension (add to .gitignore)',
+ label: () =>
+ `Ignore ${PathExt.extname(
+ this.state.selectedFile.to
+ )} extension (add to .gitignore)`,
caption: 'Ignore this file extension (add to .gitignore)',
execute: async () => {
await this.props.model.ignore(this.state.selectedFile.to, true);
@@ -357,8 +361,12 @@ export class FileList extends React.Component<IFileListProps, IFileListState> {
await this.addFile(...this.markedFiles.map(file => file.to));
};
- updateSelectedFile = (file: Git.IStatusFile | null) => {
- this.setState({ selectedFile: file });
+ updateSelectedFile = (file: Git.IStatusFile | null): Promise<void> => {
+ const stateSet = new PromiseDelegate<void>();
+ this.setState({ selectedFile: file }, () => {
+ stateSet.resolve();
+ });
+ return stateSet.promise;
};
get markedFiles() { This works. But it brings React error about virtual event handling. |
@fcollonval Thx a lot for shimming in. I have opened #712 to track the fileitem state issue. This can be solved outside of this PR. @mlucool My last commit allows to exclude I think this PR takes everyone inputs and expectations, so it is ready for review. |
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 @echarles I left a couple of comments and questions.
@fcollonval Thx a lot for your reviews. My commit implements your suggestions. |
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.
A few small nits; otherwise, LGTM.
Co-authored-by: Athan <kgryte@gmail.com>
Co-authored-by: Athan <kgryte@gmail.com>
Co-authored-by: Athan <kgryte@gmail.com>
Thx @kgryte for your reviews. I have committed your suggestions. |
@fcollonval You have any other feedback for this PR? |
/binder |
1 similar comment
This is good to go for me. What about you @kgryte ? |
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.
LGTM
LGTM. Will merge now.., |
Thx all for the inputs, reviews and for merging this. |
@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. |
…11.x Backport PR #705: Git ignore UI
Fixes #683
This add a context
Ignore
menu for untracked files. When the user clicks onIgnore
, the server is requested to append a line in the .gitignore file (will create it if it does not exist) and then the .gitignore file is opened and shown to the user.