Skip to content

Conversation

nzaytsev
Copy link
Contributor

@nzaytsev nzaytsev commented Jan 15, 2025

Description

The additional dropdown is appeared only when not there are some branches hidden. It blinks twice after the first branch hiding, then it blinks one time per hidden branch. The dropdown is closed when the list is ended

Screen.Recording.2025-01-15.at.15.38.09.mov

Checklist

  • I have followed the guidelines in the Contributing document
  • My changes follow the coding style of this project
  • My changes build without any errors or warnings
  • My changes have been formatted and linted
  • My changes include any required corresponding changes to the documentation (including CHANGELOG.md and README.md)
  • My changes have been rebased and squashed to the minimal number (typically 1) of relevant commits
  • My changes have a descriptive commit message with a short title, including a Fixes $XXX - or Closes #XXX - prefix to auto-close the issue that your PR addresses

@nzaytsev nzaytsev linked an issue Jan 15, 2025 that may be closed by this pull request
@nzaytsev nzaytsev force-pushed the feature/3101-unhide-branches-dropdown branch from 8d456cd to 0549b9f Compare January 15, 2025 08:42
@justinrobots
Copy link
Collaborator

Based on what I can see in the video, this seems like a good improvement over existing functionality.

@axosoft-ramint @d13 Is there any reason this shouldn't go into 16.2?

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

Without updating the component, the "hidden branches" UX is now in two places - in our toolbar and in the branch/tag header. I haven't checked, but if the in-header one is not implemented in the component, perhaps we could remove it. If it is in the component, we would probably want a way to disable it.

Neither of that needs to block this from merging though - just debt.

I do have some tiny nitpicks about the code itself which can be addressed before merging - see below.

@justinrobots
Copy link
Collaborator

Thanks @axosoft-ramint ! @nzaytsev if you can address Ramin's concerns here, then we can get this into the 16.2 release.

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

More thoughts/suggestions now that I've had another look at it.

Comment on lines +103 to +114
function getRemoteIcon(type: string | number) {
switch (type) {
case 'head':
return 'vm';
case 'remote':
return 'cloud';
case 'tag':
return 'tag';
default:
return '';
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be using the remote's avatar icon for a remote branch in this list if the avatars are enabled in the graph's settings. Always using a generic cloud icon feels like a regression

image

@axosoft-ramint
Copy link
Contributor

@nzaytsev Updated the branch with most of the labeling changes. We can do the remote icons in an update post-release,

@axosoft-ramint axosoft-ramint self-requested a review January 15, 2025 19:28
@axosoft-ramint axosoft-ramint merged commit 714c5ee into main Jan 15, 2025
3 checks passed
@axosoft-ramint axosoft-ramint deleted the feature/3101-unhide-branches-dropdown branch January 15, 2025 19:30
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.

Not at all apparent how to unhide hidden branches

3 participants