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

[Dialog] Add dialog with close button to demos #13845

Merged
merged 2 commits into from
Dec 7, 2018

Conversation

rfbotto
Copy link
Contributor

@rfbotto rfbotto commented Dec 6, 2018

Closes #13520

@oliviertassinari
Copy link
Member

@rfbotto I have pushed the approach one step further. Let me know what you think :)

capture d ecran 2018-12-07 a 00 58 19

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation component: dialog This is the name of the generic UI component, not the React module! labels Dec 7, 2018
@oliviertassinari oliviertassinari force-pushed the adding-close-button-to-dialog branch from fc10430 to f56362f Compare December 7, 2018 00:01
@eps1lon
Copy link
Member

eps1lon commented Dec 7, 2018

I don't think we should add demos the go against material guidelines.

It might be especially confusing since close icons for fullscreen dialogs are located on the left while this one is on the right.

@oliviertassinari
Copy link
Member

@eps1lon What would you change?

spec
capture d ecran 2018-12-07 a 09 26 49

google calendar
capture d ecran 2018-12-07 a 09 26 30

@eps1lon
Copy link
Member

eps1lon commented Dec 7, 2018

Well google engineers are also extending their designers decisions. This settles it for me then WRT to close icon.

I guess the important part here is that there would be no close action without the icon. The demo should reflect that. So replace the cancel action with something else so that the close action is not ambiguous.

@rfbotto
Copy link
Contributor Author

rfbotto commented Dec 7, 2018

@oliviertassinari Prefer it much more the way you did. Much cleaner and readable👍 Plus, i like the change to the demos buttons, makes them much more prominent. When it comes to the google material guidelines, as long as we make clear that we are getting away from the specification by allowing the customisation of the default behaviour, it should be okay, no?

@oliviertassinari oliviertassinari force-pushed the adding-close-button-to-dialog branch from f56362f to 802170a Compare December 7, 2018 20:43
@oliviertassinari oliviertassinari force-pushed the adding-close-button-to-dialog branch from 802170a to f1ebc9b Compare December 7, 2018 20:48
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 7, 2018

@eps1lon Points taken. I have removed the close button in the footer (one way to do it) add I have added warnings about the non-material demos (to remove the ambiguity) following your feedback.

@oliviertassinari oliviertassinari merged commit 29eae3a into mui:master Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dialog This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants