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

Critical bugs masthead nav #764

Merged
merged 4 commits into from Nov 2, 2018

Conversation

Projects
None yet
3 participants
@RCopeland
Member

RCopeland commented Nov 1, 2018

No description provided.

@RCopeland RCopeland requested a review from yodasw16 Nov 1, 2018

@RCopeland RCopeland requested a review from afebbraro as a code owner Nov 1, 2018

@@ -25,6 +25,7 @@ import './utilities/polyfills/StringIncludes';
import './utilities/polyfills/ArrayFind';
import './utilities/polyfills/NodeListForEach';
import './utilities/polyfills/classListSVG';
import './utilities/polyfills/focusWithin';

This comment has been minimized.

@RCopeland

RCopeland Nov 1, 2018

Member

@yodasw16 @afebbraro Do you guys think its worth it to load a polyfill for this?
If I dont, then in IE / Edge, when a user tabs into a wide-nav item's submenu, the wide-nav item wont be marked active. Mouse use is fine, this would only affect keyboard only.

This comment has been minimized.

@yodasw16

yodasw16 Nov 1, 2018

Member

They will still be able to see which submenu item they are currently on? If so, the submenu being visually under the main item should be enough to indicate which one is active. Unless I'm misunderstanding.

On the other hand, the polyfill is tiny (< 1k).

@RCopeland RCopeland closed this Nov 1, 2018

@RCopeland RCopeland reopened this Nov 1, 2018

@@ -0,0 +1,34 @@
/* eslint-disable */
(function(window, document){

This comment has been minimized.

@afebbraro

afebbraro Nov 2, 2018

Member

Should we convert this to ES6 to match the rest of our JS?

This comment has been minimized.

@RCopeland

RCopeland Nov 2, 2018

Member

ended up removing the polyfill

@RCopeland RCopeland merged commit 11cde16 into sparkdesignsystem:staging Nov 2, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@RCopeland RCopeland deleted the RCopeland:critical-bugs-masthead-nav branch Nov 2, 2018

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