Skip to content

Conversation

@DenisaCG
Copy link
Member

@DenisaCG DenisaCG commented Sep 6, 2023

In the Tags panel 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 Tag is 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 the History sidebar 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.

@welcome
Copy link

welcome bot commented Sep 6, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Binder 👈 Launch a Binder on branch DenisaCG/jupyterlab-git/new-tag

@DenisaCG
Copy link
Member Author

DenisaCG commented Sep 7, 2023

Thanks for taking a look and pointing them out! @krassowski

I'm still in the process of updating the PR.

@DenisaCG DenisaCG marked this pull request as draft September 7, 2023 10:16
@DenisaCG DenisaCG marked this pull request as ready for review September 19, 2023 21:30
Copy link
Member

@fcollonval fcollonval left a 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 tagsList you added on the toolbar component at

    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 tagsChanged to the mocked model at

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 tagsList in createProps helper

@DenisaCG
Copy link
Member Author

Thank you for your review and all the suggestions to improve the code! @fcollonval

Copy link
Member

@fcollonval fcollonval left a 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.

Copy link
Member

@fcollonval fcollonval left a 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:

await self.props.model.checkoutTag(tag);

Copy link
Member

@fcollonval fcollonval left a 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

@fcollonval fcollonval merged commit 07ee168 into jupyterlab:main Sep 25, 2023
@welcome
Copy link

welcome bot commented Sep 25, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@DenisaCG DenisaCG deleted the new-tag branch September 25, 2023 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants