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

[Toolbar] Height should be 48px for Mobile Landscape #7454

Closed
pluscubed opened this issue Jul 17, 2017 · 4 comments
Closed

[Toolbar] Height should be 48px for Mobile Landscape #7454

pluscubed opened this issue Jul 17, 2017 · 4 comments
Labels
bug 🐛 Something doesn't work component: Toolbar The React component. good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@pluscubed
Copy link

Problem description

https://material.io/guidelines/layout/structure.html#structure-app-bar

Currently, the toolbar uses the 'sm' width breakpoint, so the toolbar becomes 64px in landscape on mobile devices. According to the guidelines, it should be 48px.

Link to minimal working code that reproduces the issue

https://material-ui-1dab0.firebaseapp.com/component-demos/app-bar

  1. Use Chrome DevTools to emulate mobile device.
  2. Click the rotate button in DevTools.

toolbar-height

Versions

  • Material-UI: 1.0.0-alpha.21
  • React: 15.6.1
  • Browser: Chrome 59.0.3071.115 Windows x64
@kgregory
Copy link
Member

kgregory commented Jul 17, 2017

The issue lies in the stylesheet, which makes no direct determination of mobile/desktop, but relies on the defined breakpoints. Below small, the device is considered mobile. Small and higher, the device is considered Tablet/Desktop:

export const styleSheet = createStyleSheet('MuiToolbar', theme => ({
  root: {
    position: 'relative',
    display: 'flex',
    alignItems: 'center',
    height: 56,
    [theme.breakpoints.up('sm')]: {
      height: 64,
    },
  },
  gutters: theme.mixins.gutters({}),
}));

In order to properly detect mobile landscape, where one of the dimensions falls below the small breakpoint, we'd have to consider the device height before specifying a toolbar height of 64.

Perhaps some variant of the existing breakpoint implementation that optionally adds min-height to its media queries to constrain the other dimension, if necessary?

Or, maybe we extend theme to include a new object, devices, that works similarly to breakpoints and uses media queries to determine which device type applies (given some assumptions with the current set of breakpoints)?

Thoughts on this, @oliviertassinari?

@pluscubed
Copy link
Author

pluscubed commented Jul 18, 2017

For reference, this is how material-components-web currently implements it:

    min-height: 64px;

    @media (max-width: 959px)
      and (orientation: landscape) {
      min-height: 48px;
    }

    @media (max-width: 599px) {
      min-height: 56px;
    }

@oliviertassinari oliviertassinari added v1 component: Toolbar The React component. labels Jul 18, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2017

I wish we could rely on material-components-web at some point. But given how different their styling solution is from ours, it might be simpler to take inspiration from their style.
I like the and (orientation: landscape) solution. We can use that.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Jul 18, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2017

Sure, we could create a new abstraction. But that's gonna require some thought:

  • We need to better understand the specification. I find the responsive docs confusing.
  • How do we know the device type? User Agent based solution would quite increase the payload. Do we need that much information? I would say no.
  • Are orientation and width enough to implement the logic? Maybe
  • Do we need an abstraction on top of orientation? Given how straightforward it is, I would say no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Toolbar The React component. good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

No branches or pull requests

3 participants