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

Add material theme support #63

Merged
merged 8 commits into from
Oct 23, 2018
Merged

Conversation

alexberazouski
Copy link

@alexberazouski alexberazouski commented Oct 19, 2018

Fix lumo issue #60
Fix lumo issue when custom and default buttons look distorted


This change is Reviewable

Alex added 3 commits October 19, 2018 12:01
Fix lumo issue #60
Fix lumo issue when custom and default buttons look distorted
Copy link
Member

@jouni jouni left a comment

Choose a reason for hiding this comment

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

We are using unsupported and undocumented selectors in our own themes, which gives the wrong impression for our users (that it would be okay to rely on those). I’m talking about all the class selectors used here, like .cancel-button. We are using those in the Lumo theme as well 🙁

We should consider exposing more stylable parts for those if needed. Ideally, we could set those directly on the button elements, but that prevents from setting any flex properties on them. Not exactly sure what to do here.

Nit: would like to see PRs that focus on one issue at a time. Now it’s a bit odd, IMO, fix the Lumo theme in this same PR.

Otherwise looks good to me 👍

theme/material/vaadin-confirm-dialog-styles.html Outdated Show resolved Hide resolved
@jouni
Copy link
Member

jouni commented Oct 22, 2018

Fix lumo issue when custom and default buttons look distorted

I was wondering what this means. Can you share a screenshot what the distorted buttons look like?

@alexberazouski
Copy link
Author

@jouni In the following case cancel is a default button and the rest are custom:

screenshot 2018-10-23 at 9 50 19

@alexberazouski
Copy link
Author

alexberazouski commented Oct 23, 2018

Issue to remove the class css selectors: https://github.com/vaadin/vaadin-confirm-dialog/issues/66

@jouni
Copy link
Member

jouni commented Oct 23, 2018

@jouni In the following case cancel is a default button and the rest are custom

Yeah, that case is tricky with MD, because they don’t really have that in the specs.

Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 3 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jouni)

Copy link
Author

@alexberazouski alexberazouski left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jouni)


theme/material/vaadin-confirm-dialog-styles.html, line 14 at r1 (raw file):

Previously, jouni (Jouni Koivuviita) wrote…

Actually, I looked at the specs wrong. I think 28px is fine after all.

Done.


theme/material/vaadin-confirm-dialog-styles.html, line 19 at r1 (raw file):

Previously, jouni (Jouni Koivuviita) wrote…

Would like to avoid this, and let the “Cancel” buttons be next to the main button, aligned to the right end of the footer, like they are in MD specs.

Note, that even though our own guidelines suggest placing the dismissive action at the other end of the footer, we don’t need to follow them in the Material theme, as it has its own guidelines to follow.

Done.

Copy link
Author

@alexberazouski alexberazouski left a comment

Choose a reason for hiding this comment

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

Dismissed @jouni from 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@alexberazouski alexberazouski merged commit 2c1ba13 into master Oct 23, 2018
@alexberazouski alexberazouski deleted the feat/35-material-support branch October 23, 2018 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants