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.

Sign up for GitHub

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

Jump to bottom

Avoid the FocusTrap wrapping div by default #40

Closed
wants to merge 2 commits into from
Closed

Avoid the FocusTrap wrapping div by default #40

wants to merge 2 commits into from

Conversation

opichals
Copy link

@opichals opichals commented Aug 4, 2016

The onFocus/onBlur handling is delegated to the only HotKeys child element which gets elementClone()d in the render method.

Fixes #23

@opichals opichals force-pushed the no-wrapping-div branch 2 times, most recently from 6aa5b3e to c5b77d7 Compare August 4, 2016 13:54
@chrisui
Copy link
Contributor

chrisui commented Aug 12, 2016

FocusTrap should stick around. It's an important abstraction, it manages focus state, hotkeys manage key maps and handlers and hook into focus state.

I agree there should be no need to render an extra wrapping div though but this can be achieved another way which I'd be happy to accept.

FocusTrap should only accept one child so it can simply clone it's children as you have done here with HotKeys.

HotKeys can then have an api like <HotKeys component="div" /> if you want so you can control what it renders as (note that component must forward onFocus and onBlur props).

Note the HoC api might (subjectively) achieve this much more elegantly (again would still need to forward the mentioned props).

@opichals opichals changed the title Avoid the FocusTrap wrapping div Avoid the FocusTrap wrapping div by default Sep 6, 2016
@opichals
Copy link
Author

opichals commented Sep 6, 2016 •
edited
Loading

Thanks for the feedback. I understand, I have changed the PR so that the FocusTrap is still there.

The API change is that the FocusTrap defaults to not wrapping its contents and therefore requiring a single element children (that is unless the component property is used explicitly)

@opichals opichals force-pushed the no-wrapping-div branch 4 times, most recently from 9b80985 to 43eec23 Compare September 6, 2016 11:42
@opichals
Avoid wrapping children
fa8ffa7 
This changes the API so that by default the FocusTrap doesn't wrap
the children and therefore there must only be a single child element
under HotKeys. In such a case there is no tabIndex manipulation
unless used explicitly.

In order to wrap the contents one needs to explicitly define the
component property as follows:

  <HotKeys>
     <p>This is NOT going to get wrapped with a div</p>
  </HotKeys>

  <HotKeys component="div">
     <p>This is going to get wrapped around with a div</p>
  </HotKeys>
@opichals
Added babel-register for npm run test
dec5d3d
@MLoughry
Copy link

MLoughry commented Nov 7, 2016

I'm looking at integrating this package into my project, but this issue is a major pain point. Is there any reason not to merge this PR and generate a new release?

@MLoughry MLoughry mentioned this pull request Nov 7, 2016
Closed
8 tasks
chrisui
chrisui reviewed Feb 2, 2017
View reviewed changes
lib/FocusTrap.js
@@ -11,22 +13,52 @@ const FocusTrap = React.createClass({

getDefaultProps() {
return {
component: 'div'
component: DOCUMENT_FRAGMENT
Copy link
Contributor

Choose a reason for hiding this comment

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

is "documentFragment" a new React thing? Not familiar.

chrisui
chrisui reviewed Feb 2, 2017
View reviewed changes
lib/FocusTrap.js

const child = React.Children.only(children);
if (child.onBlur) {
child.onBlur(...args);
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling methods on children is a bit anti-react as it breaks the direction of data.

@chrisui
Copy link
Contributor

chrisui commented Feb 2, 2017

I'm gonna spend a little time on #24 tomorrow at work (finally) and I'll incorporate the work here.

Can sync post that investigation but hopefully I'll have something finished and released this weekend.

Serious apologies for the lack of activity around here!

@MikeTaylor
Copy link

Can I raise a hand and ask for this? We really need <HotKeys> to not wrap its children, as that breaks assumptions about nesting that our CSS makes.

@MikeTaylor
Copy link

Meanwhile, I am trying to get by using

<HotKeys component="documentFragment" keyMap={globalKeyMap} handlers={handlers}>

but that doesn't avoid wrapping the children, as the code in this PR had led me to hope; it just means they get wrapped in a <documentfragment tabindex="-1"> instead of <div tabindex="-1">.

Is that expected?

@JohnC-80 JohnC-80 mentioned this pull request Jun 7, 2017
Closed
@greena13
Copy link
Owner

It's looking very likely that we will be merging in #61 in favour of this pull request. Thank you for your work on this, as it no doubt informed #61 and helped arrive at an alternative solution.

@greena13 greena13 closed this Nov 15, 2017
@opichals
Copy link
Author

It is good that somebody took the responsibility for the lib maintenance.

It is also good to note that there should be no need for wrapping anymore with React v16.

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

@chrisui chrisui chrisui left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@opichals @chrisui @MLoughry @MikeTaylor @greena13