Skip to content
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

feat: git branches nesting #1571

Merged
merged 4 commits into from
Jan 25, 2024
Merged

Conversation

FezVrasta
Copy link
Contributor

@FezVrasta FezVrasta commented Jan 22, 2024

Description

Implements Git branches grouping using the first slash as prefix separator. This allows for easier navigation on large repositories that prefix branches (for example: feat/some-feature)

This PR builds on top of #1570

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

CleanShot 2024-01-22 at 4  07 39@2x

@FezVrasta FezVrasta changed the title fix: git branches nesting feat: git branches nesting Jan 22, 2024
Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Edit: See next review

@austincondiff austincondiff self-requested a review January 22, 2024 19:00
Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the SwiftLine errors before we can merge.

@Angelk90
Copy link
Contributor

@FezVrasta : Can you take a screenshot of what appears when you go to the website?

@FezVrasta
Copy link
Contributor Author

What do you mean? What website?

@Angelk90
Copy link
Contributor

@FezVrasta : If you look at the screenshot you posted it says website and a side menu should open.

@austincondiff
Copy link
Collaborator

@Angelk90 I think @FezVrasta has provided sufficient details for this PR.

@Angelk90
Copy link
Contributor

Angelk90 commented Jan 22, 2024

@austincondiff : See where it says docs, the menu on the right opens and the word docs/ggc-sponsor appears, the repetition of the word docs in that case seems useless to me, yes since I'm on docs, I wanted to see if it also happened in the other cases.

@activcoding
Copy link
Member

@FezVrasta thanks for the PR! Got some questions:

1: Can we use the popover? I really like its style:
Screenshot 2024-01-24 at 1 28 04 PM

  1. How about more symbols for branches and a coloured checkmark for the current branch?
Screenshot 2024-01-24 at 1 26 21 PM
  1. Any chance to add more details to each branch section?
Screenshot 2024-01-24 at 1 30 14 PM

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Jan 24, 2024

I fixed the linter and also removed the branch prefix from the nested dropdown entries. I also added icons to each entry to easily distinguish folders and branches.

CleanShot 2024-01-24 at 2  17 43@2x

@Angelk90
Copy link
Contributor

@FezVrasta: Great, what happens if there is only one branch and it is the selected one.

In the future I would propose to change the branch icon, I would propose like that of xcode.

@FezVrasta
Copy link
Contributor Author

@FezVrasta: Great, what happens if there is only one branch and it is the selected one.

In the future I would propose to change the branch icon, I would propose like that of xcode.

I haven't changed that behavior so it will work like it does today.

@austincondiff
Copy link
Collaborator

This looks fantastic @FezVrasta! I would agree, we have a "branch" symbol in CodeEditSymbols that you might use instead.

@FezVrasta
Copy link
Contributor Author

Done

CleanShot 2024-01-25 at 3  47 14@2x

austincondiff
austincondiff previously approved these changes Jan 25, 2024
Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @FezVrasta! 👏

@Angelk90
Copy link
Contributor

Angelk90 commented Jan 25, 2024

@austincondiff , @FezVrasta : It would be possible to change the icon here too and add the same one that was used for the branch as xcode does.
Screenshot 2024-01-25 alle 19 12 49

@FezVrasta
Copy link
Contributor Author

CleanShot 2024-01-25 at 7  31 48@2x

@Angelk90
Copy link
Contributor

@FezVrasta : Great.

@Angelk90
Copy link
Contributor

@FezVrasta : I join @activcoding's proposal, do you think it's too demanding?
image

@activcoding
Copy link
Member

@FezVrasta : I join @activcoding's proposal, do you think it's too demanding? image

I reckon the popover style is more important. We can throw in those finer details in a later PR

@austincondiff
Copy link
Collaborator

Yeah, let's merge this PR. We are scope creeping quite a bit here imo. Looks good enough to merge.

@austincondiff austincondiff self-requested a review January 25, 2024 18:49
@Angelk90
Copy link
Contributor

@activcoding : If anything, can you try to add these details yourself?

@FezVrasta
Copy link
Contributor Author

@FezVrasta thanks for the PR! Got some questions:

1: Can we use the popover? I really like its style: Screenshot 2024-01-24 at 1 28 04 PM

  1. How about more symbols for branches and a coloured checkmark for the current branch?
Screenshot 2024-01-24 at 1 26 21 PM 3. Any chance to add more details to each branch section? Screenshot 2024-01-24 at 1 30 14 PM

I used the Menu component because it has built-in keyboard support, we can definitely use the Popover but I think we'll have to build keyboard support from scratch?

@activcoding
Copy link
Member

@FezVrasta thanks for the PR! Got some questions:
1: Can we use the popover? I really like its style: Screenshot 2024-01-24 at 1 28 04 PM

  1. How about more symbols for branches and a coloured checkmark for the current branch?
Screenshot 2024-01-24 at 1 26 21 PM 3. Any chance to add more details to each branch section? Screenshot 2024-01-24 at 1 30 14 PM

I used the Menu component because it has built-in keyboard support, we can definitely use the Popover but I think we'll have to build keyboard support from scratch?

Ok, great choice going with the Menu component. Your work looks fantastic—let's get this merged! If @austincondiff adds me as a reviewer, I'd be more than happy to go through your changes.

@activcoding
Copy link
Member

@activcoding : If anything, can you try to add these details yourself?

I don't like your tone here; it feels a bit demanding. Nevertheless, I've got a lot on my plate with a few PRs in progress. Once those are sorted, I'll definitely take a look at the issue.

@Angelk90
Copy link
Contributor

@activcoding : He didn't want to be, I really liked your idea.

Copy link
Collaborator

@0xWDG 0xWDG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@austincondiff austincondiff merged commit aa0a3dc into CodeEditApp:main Jan 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants