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

Tabs: Fix usage of ripple/states mixins #1866

Closed
kfranqueiro opened this issue Jan 4, 2018 · 1 comment · Fixed by #2162
Closed

Tabs: Fix usage of ripple/states mixins #1866

kfranqueiro opened this issue Jan 4, 2018 · 1 comment · Fixed by #2162
Assignees

Comments

@kfranqueiro
Copy link
Contributor

I noticed yesterday that mdc-tabs is using the ripple/states mixins in an odd way:

.mdc-tab.mdc-ripple-upgraded {

This is weird, since the ripple/states mixins themselves are responsible for selecting on .mdc-ripple-upgraded. This sort of has the side effect of having no effect on non-upgraded ripples, but it also results in spurious selectors in some places.

We used to do something like this in mdc-list as well, which was resolved in #1737.

Moreover, I'm unsure why we're treating tabs differently between JS and CSS-only. Right now we're actually re-coloring the browser's focus outline for CSS-only tabs rather than using states, which seems like an odd thing to do (and this is the only component we style outline color in).

image

I'm planning to show this current behavior to our states designer today to confirm whether we should be using states even for the case of CSS-only tabs. If that's indeed the right thing to do, hopefully it means we can simplify tabs' styles and remove some.

@kfranqueiro
Copy link
Contributor Author

Update: Confirmed with design that CSS-only should also use states shades rather than outline. This means we shouldn't need the .mdc-ripple-upgraded selector within mdc-tab.css, and should be able to remove the outline style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants