Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: [M3-8855] - Surface Node Pool Tags #11368
feat: [M3-8855] - Surface Node Pool Tags #11368
Changes from 7 commits
808b96c
362da35
0a7eec4
bb72b61
04f38e3
2f1f293
c7ab1c4
d7b0b3a
aca837a
1dc5e11
26ecaa1
097fbff
9c07d53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
mobile.mov
We'll want to consider mobile view here, and might need some UX input. (Should we still have the ability to see tags in this view?) Currently, the tag cell will overlap with the pool id/encryption box once we get to ~500px in the viewport, and it gets worse at smaller screens (iPhone SE). Adjusting the box's width of 100% to less helps, but the tooltip still gets hidden. Maybe displaying tags under the pool id and encryption status is most readable?
Looks like there is also an existing issue where the Unlock icon has a min width of 16px, but the Lock icon does not, leading it to shrink and virtually disappear (and leave some odd spacing) behind at the smaller screen sizes. We could fix that here.
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.
@mjac0bs Good catches, should be addressed now. I moved the tags to a separate row on mobile and removed the minWidth of 16px
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.
Looks better! I think we should actually do this for tablet screen sizes too. There's an edge case with really long tags where, between tablet and mobile breakpoints, we still get some overlapping. Switching to displaying the tags below at the
md
breakpoint is also consistent with the cluster's tags. (Which seem to have their own buggy display issue... I'll create a follow up ticket to fix cluster tags. Update: M3-9009)No issues after using
md
breakpoints instead ofsm
:Screen.Recording.2024-12-11.at.12.59.02.PM.mov