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

fix(ripple): Register focus/blur handlers in IE #3294

Merged
merged 4 commits into from
Aug 8, 2018

Conversation

kfranqueiro
Copy link
Contributor

Ripple JS logic has historically always completely short-circuited browsers that don't support CSS variables. This changes it to still set up focus and blur handling, for the sake of components with complex DOM structure where a focusable element is not the root element of the ripple.

This also updates checkbox and radio styles to avoid applying their unique CSS-only focus styles to instances with JS ripples... and also fixes those CSS-only focus styles to use the same opacity as the JS ripples.

Fixes #3212.

@acdvorak acdvorak self-assigned this Aug 3, 2018
const {ROOT, UNBOUNDED} = MDCRippleFoundation.cssClasses;
requestAnimationFrame(() => {
this.adapter_.addClass(ROOT);
if (this.adapter_.isUnbounded()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Invert this if statement and return early to reduce nesting:

const isBounded = !this.adapter_.isUnbounded();
if (isBounded) {
  return;
}
this.adapter_.addClass(UNBOUNDED);
this.layoutInternal_();

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 might consider this if there were a significant number of operations inside of the condition, but it's 2 lines of code and I feel like the early bail-out on an inverted condition would be harder to read (or, in your example with a separate variable, takes 2 more LOC than what's here now).

@codecov-io
Copy link

codecov-io commented Aug 3, 2018

Codecov Report

Merging #3294 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3294   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files         120      120           
  Lines        5184     5184           
  Branches      649      650    +1     
=======================================
  Hits         5082     5082           
  Misses        102      102
Impacted Files Coverage Δ
packages/mdc-ripple/foundation.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8ded0a...6d614e5. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 176 screenshot tests passed for commit 487fd24 vs. master! 💯🎉

Copy link
Contributor

@williamernest williamernest left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@acdvorak acdvorak left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdc-web-bot
Copy link
Collaborator

All 188 screenshot tests passed for commit 9403452 vs. master! 💯🎉

@mdc-web-bot
Copy link
Collaborator

All 188 screenshot tests passed for commit 6d614e5 vs. master! 💯🎉

@kfranqueiro kfranqueiro merged commit 1186f9b into master Aug 8, 2018
@kfranqueiro kfranqueiro deleted the fix/ripple/focus-ie branch August 8, 2018 17:49
YuoMamoru pushed a commit to YuoMamoru/material-components-web that referenced this pull request Aug 14, 2018
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.

Updated Switch: Focus indicator doesn't work on IE11
5 participants