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

web: proposal - add a small show more button in each section with more than 20 items #5483

Merged
merged 2 commits into from
Feb 14, 2022

Conversation

nicks
Copy link
Member

@nicks nicks commented Feb 11, 2022

Hello @lizzthabet,

Please review the following commits I made in branch nicks/window:

5ed0743 (2022-02-11 11:12:23 -0500)
web: proposal - add a small show more button in each section with more than 20 items

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks nicks requested a review from lizzthabet February 11, 2022 16:13
@nicks
Copy link
Member Author

nicks commented Feb 11, 2022

cc @SurbhiSGupta

Screenshot from 2022-02-11 11-10-41

This is very much a "Minimum Viable Fix" UI - something that we can push out quickly without changing the "default" experience or introducing much complexity. If a section of the sidebar has more than 20 elements, cap it and add a little button. It might make sense to make the cap even smaller - like 10 elements.

wdyt?

@SurbhiSGupta
Copy link

Love the quick fix !
Here is an option to make it even lesser wordy
image

@nicks
Copy link
Member Author

nicks commented Feb 11, 2022

Screenshot from 2022-02-11 14-30-07

Copy link
Contributor

@lizzthabet lizzthabet left a comment

Choose a reason for hiding this comment

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

this looks good to me! i tested with my "hundreds of resources" example, with and without labels, and it worked well.

do you feel strongly about the 20 max items? it's an easy threshhold to change if people complain about it, though i'm mindful that the "show more" display doesn't persist if you switch views, so i could see someone getting annoyed if they want to view a resource that doesn't make the cut off.

i was just looking at the "Resource counts in last 28 days for known teams" chart we have to get a sense of how that 20 threshhold relates. about half of the teams run more than 20 resources at once on average, but because that doesn't consider how many resources are in each label group, that may not be a helpful metric!

these are more general thoughts than specific / actionable feedback.

@@ -203,6 +267,7 @@ function SidebarListSectionItems(props: SidebarSectionProps) {
resourceView={props.resourceView}
/>
))}
{showMoreItemsButton}
Copy link
Contributor

Choose a reason for hiding this comment

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

two small optional a11y improvements we could make:

  • adding a aria-describedby attribute to the sidebar items container with the id of the ShowMoreRow. (it may be a pain to conditionally add the aria link if there aren't enough resources to hide, so i'll leave that up to you!)

  • add some screen-reader only text to make the functionality clearer, so the button reads "Show More Resources (# hidden)" like this:

...Show More <SrOnly>Resources</SrOnly>
<span>{" "}({remaining}{" "}<SrOnly>hidden</SrOnly>)</span> // but ignore how illegible this code is lol

Choose a reason for hiding this comment

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

For color accessibility, please note the following color change:

Screen Shot 2022-02-13 at 1 40 57 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think that would be an accurate use of describedby?

updated the SrOnly text and colors!

function SidebarListSectionItems(props: SidebarSectionProps) {
let [maxItems, setMaxItems] = useState(defaultMaxItems)
let items = props.items
Copy link
Contributor

Choose a reason for hiding this comment

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

(tiny nit) could you rename this var so it doesn't echo the prop name, something like itemsToDisplay? it took me a second to realize this was a var that's potentially getting reassigned elsewhere, rather than the prop data.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member Author

@nicks nicks left a comment

Choose a reason for hiding this comment

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

To speak to the high-level question - nope, I don't have a strong attachment to max=20

for what it's worth - as the set of resources gets more heterogenous (with clusters and registries and k8s secrets and k8s servers and local jobs all being presented as "sidebar items"), I suspect that the sidebar won't even make sense anymore as a way to navigate them...

@nicks nicks merged commit 4b00780 into master Feb 14, 2022
@nicks nicks deleted the nicks/window branch February 14, 2022 16:23
nicks added a commit that referenced this pull request Feb 15, 2022
nicks added a commit that referenced this pull request Feb 15, 2022
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.

3 participants