-
Notifications
You must be signed in to change notification settings - Fork 378
Add new tag feature #1264
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
Add new tag feature #1264
Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
Co-authored-by: Michał Krassowski <[email protected]>
|
Thanks for taking a look and pointing them out! @krassowski I'm still in the process of updating the PR. |
fcollonval
left a comment
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 @DenisaCG
The code looks good. The suggestions are mainly about API wording and code optimization.
Regarding the failing Jest tests, there are 3 failures:
src/__tests__/test-components/Toolbar.spec.tsx:31:5 - error TS2322: Type '{ branches: Git.IBranch[] | ({ is_current_branch: true; is_remote_branch: false; name: string; upstream: string; top_commit: string; tag: string; } | { is_current_branch: false; is_remote_branch: true; name: string; upstream: string; top_commit: string; tag: string; })[]; ... 10 more ...; trans: TranslationBundle; }' is not assignable to type 'IToolbarProps'.
Property 'tagsList' is optional in type '{ branches: IBranch[] | ({ is_current_branch: true; is_remote_branch: false; name: string; upstream: string; top_commit: string; tag: string; } | { is_current_branch: false; is_remote_branch: true; name: string; upstream: string; top_commit: string; tag: string; })[]; ... 10 more ...; trans: TranslationBundle; }' but required in type 'IToolbarProps'.
You need to add a default value for the new prop
tagsListyou added on the toolbar component at
return {
TypeError: Cannot read properties of undefined (reading 'connect')
242 | }
243 | }, this);
> 244 | model.tagsChanged.connect(async () => {
| ^
245 | await this.refreshTags();
246 | }, this);
247 | model.selectedHistoryFileChanged.connect(() => {
at GitPanel.componentDidMount (src/components/GitPanel.tsx:244:23)
at fn (node_modules/enzyme/src/ShallowWrapper.js:429:22)
at Object.batchedUpdates (node_modules/@wojtekmaj/enzyme-adapter-react-17/src/ReactSeventeenAdapter.js:688:16)
at new ShallowWrapper (node_modules/enzyme/src/ShallowWrapper.js:428:26)
at Object.shallow (node_modules/enzyme/src/shallow.js:10:10)
at Object.<anonymous> (src/__tests__/test-components/GitPanel.spec.tsx:309:21)
You need to add an attribute
tagsChangedto the mocked model at
props.model = {
src/__tests__/test-components/TagMenu.spec.tsx:82:5 - error TS2322: Type '{ tagsList?: string[]; branching: boolean; pastCommits: undefined[] | Git.ISingleCommitInfo[]; logger: Logger; model: IGitExtension; trans: TranslationBundle; }' is not assignable to type 'ITagMenuProps'.
Property 'tagsList' is optional in type '{ tagsList?: string[]; branching: boolean; pastCommits: undefined[] | ISingleCommitInfo[]; logger: Logger; model: IGitExtension; trans: TranslationBundle; }' but required in type 'ITagMenuProps'.
You need to define the new attribute
tagsListincreatePropshelper
Co-authored-by: Frédéric Collonval <[email protected]>
Co-authored-by: Frédéric Collonval <[email protected]>
|
Thank you for your review and all the suggestions to improve the code! @fcollonval |
fcollonval
left a comment
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.
Hey @DenisaCG I looked quickly at the CI failures and pointed out what need to be fixed.
Co-authored-by: Frédéric Collonval <[email protected]>
fcollonval
left a comment
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 remaining issue is because the switching to a new tag is calling a dedicated method checkoutTag instead of checkout. The suggestions should fix it.
For reference:
jupyterlab-git/src/components/TagMenu.tsx
Line 311 in 17a2895
| await self.props.model.checkoutTag(tag); |
Co-authored-by: Frédéric Collonval <[email protected]>
fcollonval
left a comment
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 @DenisaCG
Congrats on the first PR in this repo


In the
Tagspanel where the user can view all existing tags, added the functionality of creating a new tag. This feature follows the workflow of creating a new branch.For this, a button
New Tagis displayed next to the tags filter. Once clicked, it opens a new dialog box, allowing the user to type the name for the tag and to choose which commit from the current branch the tag should apply to. The commit history is reusing some parts of theHistorysidebar panel components, stripped down to create a simpler version. The list can also be filtered/searched by the user by the message of the commits.