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

[Select] Fix popover width and add autoWidth prop #8307

Merged
merged 2 commits into from Sep 26, 2017
Merged

[Select] Fix popover width and add autoWidth prop #8307

merged 2 commits into from Sep 26, 2017

Conversation

leMaik
Copy link
Member

@leMaik leMaik commented Sep 21, 2017

This sets the minimum width of a Select's popover to the width of the select field and adds an autoWidth prop to disable this, similar to v0.x of material-ui.

Closes #8286

@leMaik leMaik changed the title Select popup menu [Select] Fix popover width and add autoWidth prop Sep 21, 2017
@oliviertassinari oliviertassinari added component: select This is the name of the generic UI component, not the React module! v1 labels Sep 21, 2017
oliviertassinari
oliviertassinari reviewed Sep 21, 2017
View reviewed changes
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.

It would be good to have a test and a demo for this new feature.

src/Select/SelectInput.js
@@ -343,6 +349,14 @@ class SelectInput extends React.Component<AllProps, State> {
...MenuProps.MenuListProps,
Copy link
Member

Choose a reason for hiding this comment

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

I think that this logic is broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't even touch this line. It seems reasonable to me. 👍

Copy link
Member

Choose a reason for hiding this comment

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

I have the feeling that if a user provides a MenuListProps property in MenuProps, the merge won't happen correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it will merge correctly, but I'll verify the logic.

Copy link
Member

Choose a reason for hiding this comment

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

My bad.

src/Select/SelectInput.js
@@ -343,6 +349,14 @@ class SelectInput extends React.Component<AllProps, State> {
...MenuProps.MenuListProps,
role: 'listbox',
}}
PaperProps={{
...MenuProps.PaperProps,
Copy link
Member

Choose a reason for hiding this comment

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

I think that this logic is broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

My bad.

src/Select/SelectInput.js Outdated
...MenuProps.PaperProps,
style: {
minWidth:
this.state.anchorEl != null && !autoWidth ? this.state.anchorEl.clientWidth : 0,
Copy link
Member

Choose a reason for hiding this comment

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

We might don't want to inline the logic given it start to be complexe.

@leMaik
Copy link
Member Author

leMaik commented Sep 21, 2017

It would be good to have a test and a demo for this new feature.

@oliviertassinari Indeed, but I don't know how to test the width of the component. Is this even possible without actually rendering the component in a browser?

@oliviertassinari
Copy link
Member

I don't know how to test the width of the component.

@leMaik I think that testing this in the browser will be hard to assert, the width might change from one browser to another. So for the sack of effectiveness, I would stick to a jsdom test. Yes, you can define your own width to the element with jsdom :).

@leMaik
Copy link
Member Author

leMaik commented Sep 21, 2017

@oliviertassinari Thanks, I'll look into that tonight and add tests.

@leMaik leMaik mentioned this pull request Sep 21, 2017
Merged
@oliviertassinari oliviertassinari self-assigned this Sep 23, 2017
@oliviertassinari
Copy link
Member

@leMaik I'm gonna continue this PR if needed. Hold on.

@leMaik
Copy link
Member Author

leMaik commented Sep 23, 2017

@oliviertassinari Well, I was going to continue this this weekend (today) but fine, feel free to get this done. 👍

@oliviertassinari
Copy link
Member

@leMaik I haven't started yet. In that case, I'm moving to #8274 👼 .

@oliviertassinari oliviertassinari removed their assignment Sep 24, 2017
@leMaik
Copy link
Member Author

leMaik commented Sep 24, 2017

@oliviertassinari I didn't find any way to get an element's width using JSDOM. It doesn't seem to have a layout algorithm so the widths are always zero. 😕

@oliviertassinari oliviertassinari self-assigned this Sep 24, 2017
@oliviertassinari
Copy link
Member

@leMaik I will have a look at it

@leMaik
Copy link
Member Author

leMaik commented Sep 24, 2017 •
edited

@oliviertassinari Thank you, I'm curious how that will work. 👍 I just added a demo and removed the inline minWidth calculation.

Edit: It would be awesome if the linter could be added as a pre-commit hook. 🙄

leMaik
leMaik commented Sep 26, 2017
View reviewed changes
src/Select/SelectInput.d.ts
multiple: boolean,
MenuProps?: Object,
name?: string,
autoWidth: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't autoWidth be optional?

Copy link
Member

Choose a reason for hiding this comment

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

It's optional yes.

leMaik
leMaik commented Sep 26, 2017
View reviewed changes
src/Select/SelectInput.js Outdated
@@ -14,7 +14,7 @@ export type Props = {
* If true, the width of the popover will automatically be set according to the items inside the
* menu, otherwise it will be at least the width of the select input.
*/
autoWidth?: boolean,
autoWidth: boolean,
Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

@oliviertassinari oliviertassinari merged commit 30c10d7 into mui:v1-beta Sep 26, 2017
@oliviertassinari
Copy link
Member

@leMaik Thanks

@leMaik leMaik deleted the select-popup-menu branch September 26, 2017 12:52
@leMaik
Copy link
Member Author

leMaik commented Sep 26, 2017

@oliviertassinari Nice! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

Assignees

@oliviertassinari oliviertassinari

Labels
component: select This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@leMaik @oliviertassinari