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

[AppBar] Prominent variant #14246

Closed
2 tasks done
jgoux opened this issue Jan 19, 2019 · 11 comments · Fixed by #17894
Closed
2 tasks done

[AppBar] Prominent variant #14246

jgoux opened this issue Jan 19, 2019 · 11 comments · Fixed by #17894
Labels
component: app bar This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/ new feature New feature or request

Comments

@jgoux
Copy link
Contributor

jgoux commented Jan 19, 2019

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Hello,

it would be great to have the prominent variant for the AppBar component. It's nice on desktop!

Examples 🌈

You can find an example in the official Material Components Web library : https://material-components.github.io/material-components-web-catalog/#/component/top-app-bar/prominent

You can also look at the specs : https://material.io/design/components/app-bars-top.html#anatomy

@mbrookes
Copy link
Member

mbrookes commented Jan 20, 2019

At first glance it looks like it should be trivial to implement (i.e. no significant code bloat), so no objection per-se; but I'm curious since we've had no requests up to now - do you have a use for this, or are you pointing out a gap vs the spec?

(If it's the latter, we should certainly add it to the "supported components" page as an unsupported variant, but I wouldn't rush to implement it, just because.)

@jgoux
Copy link
Contributor Author

jgoux commented Jan 21, 2019

I think it's a gap vs the spec as the prominent variant is visible and presented multiple times in them.

Also I like this variant better for desktop screens, it gives more emphasis for the page title and action items.

@taschetto

This comment has been minimized.

@joshwooding joshwooding added component: app bar This is the name of the generic UI component, not the React module! new feature New feature or request labels Apr 19, 2019
@wojtczal
Copy link

@mbrookes a use case I've encountered recently is having an app bar with quite long page title and icons/buttons on it, on a small mobile device (thinking about 320px wide). The material guidelines suggest to go for prominent app bar in such case. This is often language-specific as some language have quite long words (German or Spanish). Would it be something you could target soonish? Thanks!

@mbrookes
Copy link
Member

@wojtczal Would you like to work on it? The contributing guide will show you how to get started. It isn't as onerous as it looks.

@joshwooding joshwooding added good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/ labels Oct 1, 2019
@burtyish
Copy link
Contributor

burtyish commented Oct 10, 2019

@mbrookes, I'd like to take a stab at this one.
The spec lists these variants:

  • Regular
  • Prominent
  • Dense (desktop only)
  • Prominent dense (desktop only)

To add the enhancement described in this issue, adding a prop like variant: 'regular' | 'prominent' should suffice.

However, for extra pints I might try to match the spec.
Maybe variant: 'regular' | 'prominent' | 'dense' | 'prominent dense' would be ok? I'm not in love with 'prominent dense' though. Two words doesn't feel right.

@mbrookes
Copy link
Member

for extra pints

You sure you aren't British? 🍻 😆

I'm not in love with 'prominent dense' though.

For other components we have the dense prop. (See; https://material-ui.com/customization/density/). That would avoid the need for compound variants such as "prominent dense".

@burtyish
Copy link
Contributor

burtyish commented Oct 10, 2019

Haha, yeah I meant extra points, but pints works better! 🍻
Anyway, I like the dense prop idea. I'll start with the prominent variant and leave dense for another PR.

@burtyish
Copy link
Contributor

burtyish commented Oct 14, 2019

@mbrookes, before making any changes, I tried to achieve the prominent layout in a codesandbox, based on one of the demos. (my changes are marked by comments)
https://codesandbox.io/s/material-demo-prominent-appbar-h602s

I found that supporting the prominent bar variant would require changes in the Toolbar component, not the AppBar.

  1. Currently, Toolbar supports the prop
variant?: 'regular' | 'dense'

I would suggest changing the API to

dense?: boolean;
prominent?: boolean;

This follows your suggestion and will allow users to combine the two props as they like.
However, it is a breaking change.

  1. Although the Toolbar component can support a prominent variant, users would still have to tweak the styling on its children. See the menu button and title in the sandbox above.

I'd like to get feedback before making any changes.
WDYT of this approach?
Any suggestions?

@mbrookes
Copy link
Member

@burtyish Thanks for the CodeSandbox. That looks good.

Between the breaking change, and the need to style the children, I wonder if for now a demo in the docs based on your example would be the better option?

@burtyish
Copy link
Contributor

burtyish commented Oct 15, 2019

Between the breaking change, and the need to style the children, I wonder if for now a demo in the docs based on your example would be the better option?

@mbrookes you got it.
See my PR for docs above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: app bar This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/ new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants