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.

Sign up for GitHub

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

Jump to bottom

[Menu] maxHeight spec compliance #7378

Merged
merged 4 commits into from
Jul 8, 2017
Merged

[Menu] maxHeight spec compliance #7378

merged 4 commits into from
Jul 8, 2017

Conversation

rosskevin
Copy link
Member

@rosskevin rosskevin commented Jul 7, 2017 •
edited by oliviertassinari
Loading

The current menu maxHeight is arbitrarily set to be 250px, and this is too prescriptive for general use.

The spec states:

The maximum height of a simple menu should be one or more rows less than the view height. This ensures a tappable area outside of the simple menu with which to dismiss the menu.

Therefore, it is up to userland to set this limitation unless someone wants to create a smart height listener/implementation.

As a note, I left an empty stylesheet as withStyles requires one and classes are passed down (and tested).

This fixes a situation like this:

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@rosskevin rosskevin added bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! v1-alpha labels Jul 7, 2017
@oliviertassinari
Copy link
Member

The motivation sounds good to me

oliviertassinari
oliviertassinari requested changes Jul 7, 2017
View reviewed changes
src/Menu/Menu.js Outdated
@@ -159,7 +154,7 @@ class Menu extends Component<DefaultProps, Props, void> {
<Popover
anchorEl={anchorEl}
getContentAnchorEl={this.getContentAnchorEl}
className={classNames(classes.root, className)}
className={className}
open={open}
enteredClassName={classes.entered}
Copy link
Member

Choose a reason for hiding this comment

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

Is that a dead key?

src/Menu/Menu.js Outdated
maxHeight: 250,
},
});
export const styleSheet = createStyleSheet('MuiMenu', {});
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we need to keep it. I don't follow the motivation.

Copy link
Member Author

Choose a reason for hiding this comment

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

If entered is a dead key, then we don't need it. I'll double check. Otherwise withStyles requires a non null styleSheet to allow nested overrides.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that we need any CSS customization power on that component. Let me know :).

@oliviertassinari
Copy link
Member

Should close #7368.

@oliviertassinari oliviertassinari mentioned this pull request Jul 7, 2017
Closed
@rosskevin
Copy link
Member Author

As I started the requested change, I noticed an inconsistency and started increasing flow types, which I ended up strengthening flow coverage on several interrelated files. So the purpose is still the same, I just increased flow coverage with the additional changes.

@rosskevin
Copy link
Member Author

I have no idea how I decreased coverage, perhaps the indicator is a bit too sensitive. This is the complaint, and I'm not sure how to test (or if it is worth) testing the code concerned, so I'm inclined to merge unless there is an objection.

@rosskevin
Copy link
Member Author

After sleeping on it, it seemed to make sense to add back max height based on vh, just as the spec says - calc(100vh - 96px, where 96px is 2 * height of one item.

I see this does not solve #7368, but I think this is the right change. We should look at how to solve scrollbar not showing automatically as a separate bug.

oliviertassinari
oliviertassinari reviewed Jul 8, 2017
View reviewed changes
test/mocha.opts
@@ -1,4 +1,5 @@
--require babel-register
--require jsdom-global/register
Copy link
Member

Choose a reason for hiding this comment

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

What is this for, don't we already inject what's needed in https://github.com/callemall/material-ui/blob/v1-alpha/test/utils/dom.js?

Copy link
Member

Choose a reason for hiding this comment

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

Could be for the HTMLElement thing? Won't that break server-side rendering?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is needed for reference to instanceof HTMLElement usage in code, under mocha, HTMLElement is undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Should be good. Still, we gonna need SSR tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll either need to, or instruct the user to shim for server side rendering. e.g. var HTMLElement = typeof HTMLElement === 'undefined' ? function(){} : HTMLElement;

This solution found here

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW - I think this should be left to userland to shim, it seems to be a common issue, and no need for us to shim for the user, just the same as we don't babel-polyfill for them either.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, extending the dom.js could be even better :).

Copy link
Member

Choose a reason for hiding this comment

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

So we better control the global scope.

@rosskevin rosskevin changed the title [Menu] remove maxHeight - too prescriptive [Menu] maxHeight spec compliance Jul 8, 2017
@rosskevin rosskevin added the design: material This is about Material Design, please involve a visual or UX designer in the process label Jul 8, 2017
@rosskevin rosskevin merged commit fb2ee5a into mui:v1-alpha Jul 8, 2017
@rosskevin rosskevin deleted the menu-omit-max-height branch July 8, 2017 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: menu This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rosskevin @oliviertassinari