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

[docs-infra] Change the Diamond Sponsor block positioning on the side nav #37933

Merged
merged 9 commits into from
Jul 18, 2023

Conversation

danilo-leal
Copy link
Contributor

@danilo-leal danilo-leal commented Jul 12, 2023

After realizing there was a shaping doc about this, which was also something I was eager to do, this PR changes the Diamond Sponsor block positioning, making it glued at the bottom of the side nav. It's a change that optimizes the navigation experience rather than necessarily pleasing the sponsors 😅 definitely something to keep in mind in the chance this pull request comes to be accepted.

The previous positioning favored attention to them but it also degraded the browsing UX a bit given it was right at the top, consuming a ton of attention/space from the main items of the side nav.

Before After
Screen Shot 2023-07-11 at 22 36 21 Screen Shot 2023-07-11 at 22 36 27

https://deploy-preview-37933--material-ui.netlify.app/base-ui/getting-started/

@danilo-leal danilo-leal added docs Improvements or additions to the documentation scope: docs-infra Specific to the docs-infra product labels Jul 12, 2023
@danilo-leal danilo-leal self-assigned this Jul 12, 2023
@mui-bot
Copy link

mui-bot commented Jul 12, 2023

Netlify deploy preview

https://deploy-preview-37933--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against ba07d35

@zanivan
Copy link
Contributor

zanivan commented Jul 12, 2023

Great improvement! Plenty open room for more sponsors! ✨
I really liked the way it moves down when the docs expanded, however wouldn't it be a problem if we had more sponsors? I wonder if moving the diamond sponsor block to the right side container wouldn't solve this, as we often don't have so many items as the left container.

@danilo-leal
Copy link
Contributor Author

By "right side container" do you mean the table of contents? The bad thing about that is that the positioning of it would vary given each page has a different number of elements on the TOC (e.g. Material's text field is huge, so it wouldn't even be seen above the fold). This approach sort of allows for more sponsors (i.e. adding a new block) but it would invariably add more content to the page, the trade-off then reducing their logos a bit, which I wonder how it would affect the concept of being a diamond sponsor (parting from the assumption that one of the benefits is being clearly visible).

@zanivan
Copy link
Contributor

zanivan commented Jul 12, 2023

By "right side container" do you mean the table of contents? The bad thing about that is that the positioning of it would vary given each page has a different number of elements on the TOC

Yep, I meant the TOC 😅
I was thinking that we could use the exact same component that you've come up with, but under the TOC instead. To prevent it to be below the fold, I thought we could add a vertical scroll to the TOC, so it would roll behind the sponsor diamonds.

I made a quick draft as an example:

image

Anyway, it's just an idea—I'm not so keen on adding one more vertical scroll on the page.

@danilo-leal
Copy link
Contributor Author

Gotcha 🤙 Both the above-the-fold problem + the vertical scroll would be sort of the same on either side so it ends up being a matter of keeping it on the left or putting it on the right (TOC). I don't have a super strong stance here but the least painful way to go is just keeping on the side nav, I guess!

@samuelsycamore
Copy link
Member

This definitely feels like a better user experience, and honestly it should make the sponsors happier too since the logos are always visible.

As for moving them to the TOC side—some pages (like Autocomplete) have a lot of headers and subheaders, and we usually try to avoid creating so many that you end up with a second scrollbar right next to the main scrollbar on the page that makes it a pain to interact with both of them. So I'd be afraid moving it to that side would cause too many extra scrollbars to pop up.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

  1. The initial comment https://www.figma.com/file/MfirV6NOFAp2xxAZX1QU1V?node-id=1222:14581&mode=design#396196912.
  2. The business metric to monitor https://analytics.google.com/analytics/web/#/report/content-event-events/a106598593w159022482p160376982/_u.dateOption=last30days&explorer-segmentExplorer.segmentId=analytics.eventLabel&_r.drilldown=analytics.eventCategory:sponsor&explorer-table.plotKeys=%5B%5D/ (clicks generated, higher is better). I'm very curious to see how this change plays out, while it doesn't solve https://www.notion.so/mui-org/Change-Diamond-sponsor-location-23e4d76804084a09960393e477576096, it still has the potential to be a step ahead.
  3. Based on 2. it looks like there is one obvious problem: Doit logo is too small relative to the other logos. either to increase it or decrease the other logos.

docs/src/modules/components/DiamondSponsors.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the design This is about UI or UX design, please involve a designer label Jul 14, 2023
@danilo-leal
Copy link
Contributor Author

Just for the record: a discussion about this also happened on Figma where we considered the following possibility as well, which would enable the addition of another sponsor [image above]. There might exist other alternatives, too, so we'll probably keep exploring! @oliviertassinari suggested holding off on this option for now so we do changes more incrementally ⎯ agreed!

Screen Shot 2023-07-14 at 10 41 28

That said, @oliviertassinari, to confirm: how are we feeling about moving forward with this? That wasn't clear from your last comment! I've tweaked the dimension of the logos a bit and hopefully, it's a bit more balanced. This iteration is not yet solving the having more sponsors problem but it does feel like navigation experience improvement, which I think it's worth it! But I do wanted to see if you sign off as well.

@oliviertassinari
Copy link
Member

@danilo-leal I think it's worth experimenting with, so moving forward with. I like it decreases the click distance to the navigation. Things I think we need to address before merging:

  • Have a better balance between the visual "weight" of each logo
  • Give a lot more space on mobile to scroll the list. The Diamond sponsors takes at least 30% more space than available on my phone.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 14, 2023

I have created a report with GA4 in case UA.js stops collecting data (it's not supposed to collect since July 1st) https://www.notion.so/mui-org/Product-Analytics-f2e2576203564e6c8dc3429cf305a36f?pvs=4#2e3eb391e39a4043b559a3c04bd9e304.

@danilo-leal
Copy link
Contributor Author

@oliviertassinari Can you check again the things you mentioned (visual weight & mobile space) and let me know if you think there's any needed further change?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 15, 2023

Mobile looks good, Desktop, I would make Doit bigger, Zesty a bit bigger

Current
Screenshot 2023-07-15 at 02 01 40

So maybe we need to:

  1. Set the width/height of the source image to keep the image ration
  2. Set the height for each on mobile, and on desktop, both aiming for a specific "visual weight" but different as we have less space on mobile.

Last review cycle from my end (so I don't go too deep), feel free to ship what you feel is right.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 17, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 17, 2023
@danilo-leal
Copy link
Contributor Author

cc @oliviertassinari & all: I think this is looking good 😬 Let me know (with an approval 😅) if you agree or any other thoughts we may want to tackle before moving forward!

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 17, 2023

I pushed a quick commit to fix these bugs:

  1. Before
Screenshot 2023-07-18 at 00 33 00

After

Screenshot 2023-07-18 at 00 32 47
  1. Before
Screen.Recording.2023-07-18.at.00.34.27.mov
  1. Before
Screen.Recording.2023-07-18.at.00.36.18.mov

@danilo-leal danilo-leal merged commit 6b01133 into master Jul 18, 2023
24 checks passed
@danilo-leal danilo-leal deleted the change-diamond-sponsor-positioning branch July 18, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design This is about UI or UX design, please involve a designer docs Improvements or additions to the documentation scope: docs-infra Specific to the docs-infra product
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

None yet

5 participants