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: Fix error in #7503 PR and tidy up docs #7571

Merged
merged 1 commit into from Jun 27, 2018

Conversation

Projects
None yet
2 participants
@tofumatt
Member

tofumatt commented Jun 27, 2018

I messed up in #7503 and @afercia graciously caught it. #7503 (comment)

This fixes it and clarifies the docs. Sorry!

iu-2

^^ Me

@tofumatt tofumatt added this to the 3.2 milestone Jun 27, 2018

@tofumatt tofumatt requested review from afercia and WordPress/gutenberg-core Jun 27, 2018

@afercia

This comment has been minimized.

Contributor

afercia commented Jun 27, 2018

I think the previous syntax allowed the prop to be omitted (null) and still have focus managed by default, which may not be necessary any longer now that there's a default prop value, right?

Aside: I've missed why Gutenberg rarely uses defaultProps, pretty sure there's a reason I just don't know about it 🙂.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 27, 2018

I'm at a loss as to why. Actually the component was using a kinda of hacky defaultProp… just setting defaults in the object deconstruction:

const { getAnchorRect = this.getAnchorRect, position = 'top', expandOnMobile } = this.props;

🤷‍♂️

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 27, 2018

As mentioned in the issue, this is using defaultProps now so not passing the prop sets it as "firstElement": #7503 (comment)

@afercia

This comment has been minimized.

Contributor

afercia commented Jun 27, 2018

Thanks @tofumatt. One unintended consequence of #7503 is that all the tooltips on all buttons etc. require now two tab presses to be navigated:

  • first tab press focuses the button, the tooltip appears
  • second tab press: the tooltip unmounts and FocusManaged moves focus back to the element that invoked the tooltip (which is the button itself)
  • third tab press: finally navigates away from the button

This PR fixes it 👍

@tofumatt tofumatt merged commit a5d6a53 into master Jun 27, 2018

2 checks passed

codecov/project 46.78% (+0%) compared to e197ba0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the fix/tofumatt-dunce-hat branch Jun 27, 2018

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