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

GrafanaUI: Smaller padding around Drawer's title, subtitle, and tabs #75354

Merged
merged 9 commits into from Sep 29, 2023

Conversation

polibb
Copy link
Contributor

@polibb polibb commented Sep 25, 2023

What is this feature?

The whole sidebar of Save dashboard (modal > drawer > save dashboard form) is scrollable.

EDIT: After discussion we agreed to leave the title, subtitle, and tabs always visible and scroll only the content, as it was until now. This PR is removing the option to make the content not scrollable: #75287

This PR only tweaks the padding on smaller screens, so that it's easier to access the content.

Why do we need this feature?

Because otherwise when the page is zoomed-in the user cannot see the input for the description and Save/Cancel buttons well, even though the content itself is scrollable. The title and subtitle above the content take up too much space.

Who is this feature for?

People with lower vision abilities, who need to zoom in the page a lot to be able to read it.

Which issue(s) does this PR fix?:

Fixes #66490

Special notes for your reviewer:

Not sure this is the right approach, but it's an approach. Feel free to suggest other solutions, if you think you have something better.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@polibb polibb added area/dashboard area/frontend add to changelog type/accessibility Accessibility problem / enhancement no-backport Skip backport of PR wcag/1.4.10 Content can be presented with no loss to info or functionality and without two-dimensional scroll labels Sep 25, 2023
@polibb polibb added this to the 10.2.x milestone Sep 25, 2023
@polibb polibb requested a review from a team as a code owner September 25, 2023 09:10
@polibb polibb self-assigned this Sep 25, 2023
@polibb polibb requested a review from a team as a code owner September 25, 2023 09:10
@polibb polibb requested review from ivanortegaalba, kaydelaney, tskarhed and eledobleefe and removed request for a team September 25, 2023 09:10
@polibb polibb changed the title Polibb/a11y scroll dashboard sidebar Dashboard: Form under 'Save dashboard' blade is getting truncated after setting the view port at 320*256. Sep 25, 2023
@polibb polibb changed the title Dashboard: Form under 'Save dashboard' blade is getting truncated after setting the view port at 320*256. Dashboard: Everything under 'Save dashboard' can be scrolled when not visible Sep 25, 2023
@joshhunt
Copy link
Contributor

Maybe @ashharrison90 has fixed this already? #75287

@polibb
Copy link
Contributor Author

polibb commented Sep 25, 2023

@joshhunt, nope - just checked. What @ashharrison90 is doing is removing the option to make the content scrollable and making it scrollable by default. I'm making the all of the Save dashboard sidebar scrollable, because when you only scroll the content it's still very difficult to use on smaller window (higher zoom).

But also I'm making the space on smaller screens more compact, so the title and subtitle don't take up most of the space anyway.

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

@polibb sorry for the delay, wanted to chat to the team in our weekly about this.

i don't think we should allow for scrolling all of the drawer content tbh. it allows for scrolling off screen things which feel quite important to keep visible (drawer title, description, tabs, etc.)

however i think the tweaks you've done to header padding are probably worth exploring further, even at full screen it seems rather large. wdyt about a combination of my PR (to always scroll the content) and some tweaks to padding in this PR, maybe working with @staton-hyse11 or @amy-super to make sure they're happy? 🤔

@polibb
Copy link
Contributor Author

polibb commented Sep 26, 2023

thank you for taking a look here, @ashharrison90!

I see your point, but I think it would be best if there is a way to access the content and tabs without seeing the title and subtitle, at least for a bit while you're editing or reading the content itself (something like in-focus out-of-focus movement to center the content for you) when you're on such a small screen.

Now if we merge both, having made the content mandatorily scrollable will bring us back where it's difficult to see it on a screen 320x256, as described in the a11y issue. So yeah, I can tweak the padding a bit more above the tabs 😊.

Maybe we can tweak the padding and font for all screen sizes, if it's alright with @staton-hyse11, @amy-super, or @Ijin08 ?

@ashharrison90
Copy link
Contributor

thank you for taking a look here, @ashharrison90!

I see your point, but I think it would be best if there is a way to access the content and tabs without seeing the title and subtitle, at least for a bit while you're editing or reading the content itself (something like in-focus out-of-focus movement to center the content for you) when you're on such a small screen.

Now if we merge both, having made the content mandatorily scrollable will bring us back where it's difficult to see it on a screen 320x256, as described in the a11y issue. So yeah, I can tweak the padding a bit more above the tabs 😊.

Maybe we can tweak the padding and font for all screen sizes, if it's alright with @staton-hyse11 and @amy-super?

it's difficult to see anything at 320x256... 😅 that's just not really a screen size the product can work at. i'd rather we optimise for a more standard resolution and do as much as possible to help those lower resolutions without impacting the behaviour elsewhere.

think tweaking the padding in all cases is a good shout 👍 maybe try that and dump some screenshots to see what they think

@polibb
Copy link
Contributor Author

polibb commented Sep 26, 2023

Suggestion for the design changes (only padding is tweaked):

Before:

image

After:

image

@polibb polibb changed the title Dashboard: Everything under 'Save dashboard' can be scrolled when not visible Dashboard: Smaller padding on Save dashboards sidebar when smaller screen Sep 28, 2023
Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

👋🏾 The padding changes look good, however, it would be great if we double-check with @amy-super and @staton-hyse11 :)

Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

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

i'd be in favour of just lowering padding everywhere tbh, not just at the sm breakpoint 👍

@amy-super
Copy link

I wouldn't be a UX if I didn't ask a semi-related question here 😉
Why do we show the tab at all when we only have a single tab? We don't do that on other pages, and it would be another way to save space in smaller viewports - especially if it's just labeling something as vague as "Details" as in this example. WDYT about also making it so the tab only appears if there are multiple?

But generally, I'm cool with how this looks. I'm pretty sure we don't have a pattern for this component yet in Figma, so we'll want to add it to the DS backlog somehow to get it added with the new padding. @staton-hyse11 how do we do that?

@staton-hyse11
Copy link
Contributor

@polibb This change looks good to me 👍. I also agree with @ashharrison90 on this. I think adjusting the padding for all breakpoints makes sense here.

@amy-super, getting this into Figma should be straightforward if there is someone to do the design work. @polibb are you working with a designer? if not, we can create an issue and drop in our backlog for future work and someone to pickup.

@polibb
Copy link
Contributor Author

polibb commented Sep 28, 2023

@amy-super That's a good question - I guess no one has thought about the case where it's just one tab, probably because it's only one tab when there are no changes and if there are no changes there's no point to be on this screen at all - you cannot click 'Save'.

Thanks, @staton-hyse11, I'll adjust the padding on all screens here then.
On this specific issue no designer is involved on my side, so please create an issue 🙏 😊.

@staton-hyse11
Copy link
Contributor

staton-hyse11 commented Sep 28, 2023

Saga backlog issue created: #75693

@polibb polibb enabled auto-merge (squash) September 29, 2023 09:19
@polibb polibb changed the title Dashboard: Smaller padding on Save dashboards sidebar when smaller screen Dashboard: Smaller padding around Drawer's title, subtitle, and tabs Sep 29, 2023
@polibb polibb changed the title Dashboard: Smaller padding around Drawer's title, subtitle, and tabs GrafanaUI: Smaller padding around Drawer's title, subtitle, and tabs Sep 29, 2023
@polibb polibb merged commit d55c12c into main Sep 29, 2023
16 checks passed
@polibb polibb deleted the polibb/a11y-scroll-dashboard-sidebar branch September 29, 2023 11:21
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
…reen (#75354)

* content not scrollable

* not able to scroll the whole drawer

* reduce space inbetween drawer's title, subtitle, and tabs
@torkelo
Copy link
Member

torkelo commented Oct 2, 2023

@staton-hyse11 @amy-super I think the top padding removal looks ok but no padding between subtitle and tabs looks very cramped.

The latest change (No padding between subtitles and tabs):
Screenshot from 2023-10-02 15-44-06

With 8px padding below the subtitle:
image

@staton-hyse11
Copy link
Contributor

That looks good 👍

mildwonkey pushed a commit that referenced this pull request Oct 4, 2023
…reen (#75354)

* content not scrollable

* not able to scroll the whole drawer

* reduce space inbetween drawer's title, subtitle, and tabs
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/dashboard area/frontend no-backport Skip backport of PR type/accessibility Accessibility problem / enhancement wcag/1.4.10 Content can be presented with no loss to info or functionality and without two-dimensional scroll
Projects
Archived in project
8 participants