Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

fix(dialog): Conform more closely with spec #3575

Merged
merged 3 commits into from
Sep 19, 2018
Merged

Conversation

kfranqueiro
Copy link
Contributor

@kfranqueiro kfranqueiro commented Sep 17, 2018

Note: This is based on the dialog-stacked-reverse branch since that already has other golden updates.

This fixes the following:

  • Increases spacing between top of dialog and body text when there is no title
  • Reduces spacing between title and body text to get as close to spec as possible for alert dialogs
    • In particular, however, the specs WRT simple vs. alert dialogs seem to contradict each other in terms of how much space is between title and content
  • Reduces spacing when dialog is scrollable to match spec
    • Also eliminates padding from MDC List for confirmation dialogs, since Dialog styles already add content padding

I've updated goldens, but I think it's out of sync, because the screenshot test report showed "added" chips screenshot tests which are already in master. I'll have to straighten that out after the other dialog PRs get merged.

@kfranqueiro kfranqueiro changed the base branch from feat/dialog-stacked-reverse to master September 18, 2018 15:01
@kfranqueiro kfranqueiro added this to the Dialog milestone Sep 18, 2018
@@ -208,6 +208,25 @@

// postcss-bem-linter: end

.mdc-dialog__title + .mdc-dialog__content {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move these two style rules next to .mdc-dialog__content above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try that, but I suspect I'll have to exclude them from bem-linter anyway and might also get increasing-specificity warnings. That's why I put them at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, had to exclude from bem-linter. No other problems though. Also apparently had a HTML lint error that I fixed.

@kfranqueiro kfranqueiro merged commit 359710d into master Sep 19, 2018
@jamesmfriedman jamesmfriedman mentioned this pull request Sep 26, 2018
49 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants