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

Input in allotment pane does not auto focus using el.focus in a useEffect #402

Closed
adri1wald opened this issue Aug 22, 2022 · 5 comments · Fixed by #404
Closed

Input in allotment pane does not auto focus using el.focus in a useEffect #402

adri1wald opened this issue Aug 22, 2022 · 5 comments · Fixed by #404
Assignees
Labels
bug Something isn't working

Comments

@adri1wald
Copy link

Hey John!

I ran into an issue auto-focusing an input in one of our components that is rendered in allotment.

I narrowed down the issue to the split view view being display: none at the time the component renders. This causes el.focus to noop.

I produced a reproduction, which you can find at https://github.com/adri1wald/allotment-focus-issue-repro. Interestingly the issue does not occur when you mount <Allotment /> with defaultSizes

Best,
Adrien

@adri1wald
Copy link
Author

adri1wald commented Aug 22, 2022

For now I've discovered that I can work around the issue in our cases by doing:

  // big value makes all panes equal size
  <Allotment defaultSizes={new Array(panes.length).fill(100000)}>
    ...
  </Allotment>

Edit:
This only works for when mounting. It seems the issue is broader. Specifically, when I add a new pane it also isn't visible at the time I try to focus. I noticed that if I changed the "Add, remove or update views as children change" useEffect to a useLayoutEffect this is fixes the issue for adding new panes

@johnwalley johnwalley self-assigned this Aug 23, 2022
@johnwalley johnwalley added the bug Something isn't working label Aug 23, 2022
@johnwalley
Copy link
Owner

johnwalley commented Aug 23, 2022

Thanks for the report and reproduction. I agree it's not desirable behaviour. I'll get it fixed!

I'll revisit the use of useLayoutEffect while I'm at it. It would be nice to use useEffect for simplicity (I honestly can't remember why it's there!).

Edit: I read your comment the wrong way round. More useLayoutEffect helps I see.

@johnwalley johnwalley linked a pull request Aug 24, 2022 that will close this issue
@johnwalley
Copy link
Owner

I threw together a PR just now: #404. I need to think through the consequences but I think losing display: none should be fine. The panes have overflow: hidden so no content should show through.

I've also changed adding, updating and removing panes happen in a useEffectLayout now.

@adri1wald
Copy link
Author

Will give it a go!

@adri1wald
Copy link
Author

Heya John, just wondering when this change will be released through npm?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants