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

feat(tab-bar): Add focusOnActivate flag #3748

Merged
merged 3 commits into from
Oct 15, 2018

Conversation

kfranqueiro
Copy link
Contributor

Fixes #3585.

This also adds docs and tests I apparently missed for useAutomaticActivation, and removes an unused property back from before event registration was moved to the components.

@codecov-io
Copy link

codecov-io commented Oct 12, 2018

Codecov Report

Merging #3748 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3748      +/-   ##
==========================================
+ Coverage   98.47%   98.51%   +0.04%     
==========================================
  Files         120      120              
  Lines        5232     5239       +7     
  Branches      657      658       +1     
==========================================
+ Hits         5152     5161       +9     
+ Misses         80       78       -2
Impacted Files Coverage Δ
packages/mdc-tab-bar/index.js 86.95% <100%> (+3.37%) ⬆️
packages/mdc-tab/foundation.js 100% <100%> (ø) ⬆️
packages/mdc-tab/index.js 96.36% <100%> (+0.13%) ⬆️

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 ebb3d6f...e5111ec. Read the comment docs.

@mdc-web-bot
Copy link
Collaborator

All 557 screenshot tests passed for commit 8dc5c95 vs. master! 💯🎉

@@ -102,7 +110,9 @@ class MDCTabFoundation extends MDCFoundation {
this.adapter_.setAttr(strings.ARIA_SELECTED, 'true');
this.adapter_.setAttr(strings.TABINDEX, '0');
this.adapter_.activateIndicator(previousIndicatorClientRect);
this.adapter_.focus();
if (this.focusOnActivate_) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be tempted to make this a parameter for the activate function that can be passed by the TabBar. Then, focusOnActivate won't need to be stored in each Tab instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

So TabBar would pass it's value for focusOnActivate to a Tab via the activate method. I realize this makes more changes to the Tab foundation interface. If you're trying to prevent breaking changes, we can roll with the existing solution. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your idea is the first idea I had, but I thought against it for a couple of reasons:

  • It would make the activate API more awkward, given that it already has an optional ClientRect parameter
  • Semantically, it seems unlikely that you'd want to call this with different values across the same instance's lifetime; you'd probably just want to set the behavior once and roll with it

It wouldn't necessarily be a breaking change that way, since it could also be optional with a default.

Copy link
Contributor

Choose a reason for hiding this comment

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

True true, those are all good points.

@mdc-web-bot
Copy link
Collaborator

All 557 screenshot tests passed for commit e5111ec vs. master! 💯🎉

@kfranqueiro kfranqueiro merged commit 313618a into master Oct 15, 2018
@kfranqueiro kfranqueiro deleted the feat/tab-activate-optional-focus branch October 15, 2018 19:23
@jamesmfriedman jamesmfriedman mentioned this pull request Oct 30, 2018
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants