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] Avoid confusing nav items with disabled items #23283

Merged
merged 1 commit into from
Nov 8, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 27, 2020

Reverts the redesign of links in #22780 until #23444 is resolved.

The new styling of the nav items make them indistinguishable from disabled items. This is especially noticeable on top level links like "premium themes" and "blog". It doesn't really make sense to go against an existing design system.

next:
screenshot of "premium themes"-link next to collapsible menu buttons

It's less noticeable on dark themes but the general issue exists there as well.

The argument for the redesign was that it's closer to the drawer in material.io but it was not specified why that is desired. Especially because it's simply used as an inspiration from a site that is black and white and therefore is not suited to copy only certain features. Maybe material.io is never using disabled items and can therefore appropriate those styles. Maybe the use a different styling for disabled items. But I don't think they intended to use "disabled"-styles for "inactive"-styles. The actual demos of a drawer do not grey-out inactive items.

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Oct 27, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 27, 2020

No bundle size changes

Generated by 🚫 dangerJS against 9360b81

@oliviertassinari
Copy link
Member

It's definitely a challenging tradeoff, use the same color and the navigation nesting can be confusing (at least for me). Use a light font-weight and the links don't look great (at least for me). A couple of thoughts:

If we want to move in this direction, I think that the selected page should be font-weight bold, like on Gmail, this would look better. I'm personally leaning toward the current look but I think that if @mui-org/core-team finds the proposal better, we should change it, it's meant to work for the many.


On a related note, there is one aspect of the navigation that I think that we could improve. I don't think that the subsections like "Layout", "Lab", "Data display" needs to have a nested layer, we could have them flat as subheader (e.g. "Base de données"):

Capture d’écran 2020-10-27 à 23 56 21

@eps1lon
Copy link
Member Author

eps1lon commented Oct 28, 2020

material.io/'s navigation doesn't follow its own Drawer navigation guidelines.

I'm not sure you're understanding Material guidelines. You can use a different theme with different colors and typography which material.io is doing (just like we're doing with our docs). material.io not copying the default guidelines is not an indicator that the guidelines are flawed.

Does the design on stripe.com/docs/js or stripe.com/docs/payments/accept-a-payment feel as the links are disabled?

Yes, but I also don't know their design system. You can't cherry-pick from other design systems. That simply does not work.

material.io/components/navigation-drawer#anatomy encourage font-weight medium

I'd be happy with removing any distinction in the typography. I don't find it useful.

@mbrookes
Copy link
Member

Based purely on personal preference, I find the bold text on the selected nav item easier to read. No issue with the other changes.

Subsection headers rather than nesting makes sense.

@eps1lon
Copy link
Member Author

eps1lon commented Oct 29, 2020

I find the bold text on the selected nav item easier to read

I guess this is a separate discussion how we want to handle selected vs unselected state. It would be weird though to break out of the design system here specifically. As far as I know we never change the font-weight on selected items.

@oliviertassinari oliviertassinari added the waiting for 👍 Waiting for upvotes label Oct 29, 2020
@eps1lon eps1lon removed the waiting for 👍 Waiting for upvotes label Oct 29, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Oct 29, 2020

oliviertassinari added the waiting for +1 label 1 minute ago

Why would we do that? The argument I made is pretty clear. If you disagree at certain points please tell me so. But cherry-picking design-systems is not OK.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 29, 2020

The rationale is for the label is so that we optimize for the many. If we have a majority from the core team that finds that the variation works better, then let's definitely change it. Otherwise, better waiting. The dimension on which I think the change is not helping:

  • Visual recognition of what's a link and what's a button. The use of color primary and secondary was meant to create more contrast between the two.
  • The specification, MD seems to encourage a bolder font-weight for the menu.

Regarding disabled items, I don't understand why it's important to optimize for it as links can't be disabled. It's also using the secondary text design token, not the disabled one, these colors are different (but close).

@mbrookes
Copy link
Member

I guess this is a separate discussion how we want to handle selected vs unselected state.

Sure. I wasn't trying to diverge from the disable / not disabled focus of this PR, it was just noticeable to me that the blue-on-blue text at the lower font-weight is less legible than before. I will pipe down 🙊

@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Oct 30, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Nov 5, 2020

If we have a majority from the core team that finds that the variation works better, then let's definitely change it. Otherwise, better waiting.

Could you explain to me why this didn't apply to the original change? Why did you ask on twitter?

Visual recognition of what's a link and what's a button.

That is not your opinion though so I'm confused why you'd make that argument. Otherwise why allow <Button component="a" />?

Regarding disabled items, I don't understand why it's important to optimize for it as links can't be disabled.

Same as above: Why did you implement the prop for anchors then?

That's all just very convenient cherry-picking of arguments depending on what end results you want. I know that reasoning people out of positions is hard if they didn't reason themselves into them but I'll keep this open until the dissonance is recognized.

@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Nov 5, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 5, 2020

the original change

@eps1lon From what I understand in the original changes (#22780), no opposite point of view were raised related to the change of look. If it was the case, I think that it should have required the same treatment. Matt raised an issue with the lack of hover and focus-visible feedback that was fixed.

Why did you ask on twitter?

I have open https://twitter.com/olivtassinari/status/1323940410054615041 because no other core team members have really raised a preference here. This was meant to resolve if we should move forward or not. I think it answers the question. We shouldn't move forward as it's a step backward, we need to find a different solution.

Visual recognition of what's a link and what's a button.
That is not your opinion though

This is my opinion. This a dimension that was meant to be optimized for in #22780, I was frequently "lost" in the navigation. I needed to pause a second to figure out at which level I was, was I on a leaf?

Otherwise why allow
Why did you implement the prop for anchors then?

By anchor, do you mean the Link component? By prop, do you mean the component prop?

I think that this mixes two different subjects:

  • The look of the documentation and to some extent (MD constraints) the look of components, isn't about a personal preference but about works best with the majority.
  • There is a balance to find between forbidding a pattern that designers want to implement and the one we know are harmful. The context often plays an important role. At some point, when designers build mockups with buttons that look like link a link that looks like a button, you can't do anything about it, you need to support it or they will find the UI library that does.

That's all just very convenient cherry-picking of arguments depending on what end results you want.

Considering the topic is visual, and heavily rely on a feeling, I can definitely understand why you come to this conclusion. I think that from somebody else's point of view, your argument will have the same effect. I would simply conclude with:

@eps1lon
Copy link
Member Author

eps1lon commented Nov 8, 2020

From what I understand in the original changes (#22780), no opposite point of view were raised related to the change of look.

I missed this because you didn't properly describe your change. Instead you just dumped two screenshots.

This is my opinion. This a dimension that was meant to be optimized for in #22780, I was frequently "lost" in the navigation. I needed to pause a second to figure out at which level I was, was I on a leaf?

So when did this opinion change and why don't you propose removing the distinction from the core components?

@eps1lon eps1lon reopened this Nov 8, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Nov 8, 2020

Merging until we have properly discussed this e.g. in #23444. PRs should not apply an inconsistent state to documentation.

@eps1lon eps1lon merged commit b6e84b7 into mui:next Nov 8, 2020
@eps1lon eps1lon deleted the docs/disabled-nav branch November 8, 2020 11:23
oliviertassinari added a commit that referenced this pull request Nov 8, 2020
This reverts commit b6e84b742cdb0b1489bbc3d33a2d5e8ba7d20t pc71.
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants