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

[Dialog] Add support for the Alert #4022

Merged
merged 1 commit into from
Apr 19, 2016
Merged

[Dialog] Add support for the Alert #4022

merged 1 commit into from
Apr 19, 2016

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 17, 2016 •
edited by mbrookes
Loading

  • 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).

Follow the specification:
capture d ecran 2016-04-17 a 12 00 30
capture d ecran 2016-04-17 a 12 00 40

@oliviertassinari oliviertassinari added PR: Needs Review design: material This is about Material Design, please involve a visual or UX designer in the process labels Apr 17, 2016
@mbrookes
Copy link
Member

Nice! 👍

mbrookes
mbrookes reviewed Apr 17, 2016
View reviewed changes
docs/src/app/components/pages/components/Dialog/ExampleAlert.js
<RaisedButton label="Alert" onTouchTap={this.handleOpen} />
<Dialog
actions={actions}
modal={false}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be true instead?

"requiring acknowledgement" says to me the user shouldn't just be able to click away or press Esc.

Copy link
Member Author

@oliviertassinari oliviertassinari Apr 17, 2016 •
edited by mbrookes
Loading

Choose a reason for hiding this comment

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

Good question. I have just tested the Calendar google app on my Android phone.
It's behaving like modal={false}.
However, Google isn't following the Material spec at 100% in all its applications. That behavior could be wrong.

@nathanmarks
Copy link
Member

Nice one. Although I'm surprised that we had to add "support" for this. Can't believe that it wasn't possible before! wow!

@oliviertassinari
Copy link
Member Author

Can't believe that it wasn't possible before! wow!

It was supported but not documented. This PR broke it #4001. It was a good occasion to follow more closely the spec.

@nathanmarks
Copy link
Member

@oliviertassinari Gotcha.

Random thought: But it would be nice in the future if Dialog was more composable with components such as <DialogActions /> (similiar to how there is a component for Card to place actions at the bottom of the card in line with spec).

@oliviertassinari
Copy link
Member Author

@nathanmarks I'm 💯 with you on the composability.

@nathanmarks nathanmarks merged commit f34ddf0 into mui:master Apr 19, 2016
@oliviertassinari oliviertassinari deleted the dialog-alert branch April 19, 2016 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.
3 participants
@oliviertassinari @mbrookes @nathanmarks