/gutenberg

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Pass down focus prop in Toolbar component #1007

Merged
merged 1 commit into from Jun 2, 2017

Conversation

Reviewers

@BE-Webdesign BE-Webdesign

@youknowriad youknowriad

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@iseulde @aduth @BE-Webdesign
@iseulde
Member

iseulde commented Jun 2, 2017 • edited by BE-Webdesign

Closes #975. Props @BE-Webdesign!
@iseulde iseulde Pass down focus prop in Toolbar component

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
continuous-integration/travis-ci/push — The Travis CI build passed
Details
dbc9f17
@iseulde
Member

iseulde commented Jun 2, 2017

I'm not sure why the linter didn't catch this.

@iseulde iseulde requested a review from youknowriad Jun 2, 2017

@aduth
Member

aduth commented Jun 2, 2017

I'm not sure why the linter didn't catch this.

Was just about to ask the same thing 😄
@BE-Webdesign
Collaborator

BE-Webdesign commented Jun 2, 2017

Yeah, no idea, good thing we are starting to add tests in.
@aduth
Member

aduth commented Jun 2, 2017

Guessing it was considering it as the window global focus?

https://developer.mozilla.org/en-US/docs/Web/API/Window/focus
@BE-Webdesign
Collaborator

BE-Webdesign commented Jun 2, 2017

Guessing it was considering it as the window global focus?

That sounds like the culprit.
@iseulde
Member

iseulde commented Jun 2, 2017 • edited

Huh, didn't even know that existed.
@iseulde
Member

iseulde commented Jun 2, 2017

I'm all for removing the browser setting and explicitly getting from the window object...

@iseulde iseulde merged commit db7431d into master Jun 2, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@iseulde iseulde deleted the fix/toolbar-focus branch Jun 2, 2017

@aduth
Member

aduth commented Jun 2, 2017

I'm all for removing the browser setting and explicitly getting from the window object...

Worth exploring. Are window and document considered permitted only by that env flag though? Webpack remaps global as window if I recall correctly, so at worst we'd have global.getComputedStyle, global.document.querySelector, etc.
@iseulde
Member

iseulde commented Jun 2, 2017

Actually, I removed the browser flag, and it still does not error.
@aduth
Member

aduth commented Jun 2, 2017 • edited

Might be picking up default from eslint-config-wordpress, which we extend:

https://github.com/WordPress-Coding-Standards/eslint-config-wordpress/blob/25114c9/index.js#L4

In which case we'd need to explicitly set it to false.

@iseulde iseulde referenced this pull request Jun 2, 2017

Merged

eslint: remove browser env #1008

@iseulde
Member

iseulde commented Jun 2, 2017

@aduth Perfect! :)

@aduth aduth referenced this pull request Aug 4, 2017

Merged

Image Block: Adding image resizing handles #2213

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