Skip to content

Fix not focusing the selected value on menu open #3868

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

Merged
merged 1 commit into from
Mar 22, 2020

Conversation

Tirzono
Copy link
Contributor

@Tirzono Tirzono commented Nov 25, 2019

Bug

When opening the react select menu, the focused option is not scrolled into the view:

https://codesandbox.io/s/autumn-wind-l3ed8

Fix

The first effort was made in PR #3813, but since the author of the PR is not responding anymore, I decided to have a look myself and try to fix this issue. It turns out that commit 8ee3cea in PR #3569 was breaking the autofocus on menu opening, as the menuOptions in the openMenu was empty, because it was only set after onMenuOpen was called (and thus UNSAFE_componentWillReceiveProps). Now we set the menuOptions in openMenu already.

I've made buildMenuOptions memoized, so that it does not run the function again if the props that the function uses didn't change.

@changeset-bot
Copy link

changeset-bot bot commented Nov 25, 2019

🦋 Changeset is good to go

Latest commit: 616accf

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Tirzono
Copy link
Contributor Author

Tirzono commented Nov 25, 2019

I am not experienced enough with Flow and I have no clue how to solve the flow check. It's probably something simple, so let me know how to fix this issue (or feel free to edit my PR).

@Tirzono
Copy link
Contributor Author

Tirzono commented Dec 3, 2019

I can remove the memoizing part to make the build pass, if that would speed up the merging and reviewing of this PR. It would be a bit more inefficient, but it would fix the bug.

@mathieulj
Copy link

The flow errors can be resolved as follows (* note this may not follow best practices):

-      (newArgs: [Props, OptionsType], lastArgs: [Props, OptionsType]) => {
-        const [newProps, newSelectValue] = newArgs;
-        const [lastProps, lastSelectValue] = lastArgs;
+      (newArgs: any, lastArgs: any) => {
+        const [newProps, newSelectValue] = (newArgs: [Props, OptionsType]);
+        const [lastProps, lastSelectValue] = (lastArgs: [Props, OptionsType]);
-  buildMenuOptions(props: Props, selectValue: OptionsType): MenuOptions {
+  buildMenuOptions = (props: Props, selectValue: OptionsType): MenuOptions => {
...
-  }
+  };

@Tirzono
Copy link
Contributor Author

Tirzono commented Dec 16, 2019

I can see why it would not follow the best practices, but it at least it passes the build now. Up to @JedWatson to decide if he can live with this. Thank you for your help @mathieulj.

@Tirzono
Copy link
Contributor Author

Tirzono commented Jan 3, 2020

Or maybe @mitchellhamilton wants to have a look at it? :)

Make sure menuOptions is set in menuOpen to properly set the option to focus. Also make buildMenuOptions memoized to optimize calls.
@Tirzono
Copy link
Contributor Author

Tirzono commented Feb 21, 2020

Hi @flexdinesh, as per your comment in #3910, would you mind have a look at this PR? :)

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@berghall
Copy link

This is a must-have!

@akomatsu-indeed
Copy link

@JedWatson @Tirzono , any movement here? this bug is blocking us from upgrade to 3.0.8. And 3.0.3 is using componentWillReceiveProps() which is bad too.

@Tirzono
Copy link
Contributor Author

Tirzono commented Mar 19, 2020

@akomatsu-indeed unfortunately not. The PR is ready to be merged. @flexdinesh mentioned in #3910 that he will have a look at it, but since then it's silent again. For now you can use react-select-reborn (as discussed in #3910 too):

npm i react-select-reborn

The fix has been merged to that fork. Until the maintainers will be back, please use that fork.

@akomatsu-indeed
Copy link

Thanks a lot @Tirzono

@flexdinesh flexdinesh self-assigned this Mar 22, 2020
@flexdinesh flexdinesh merged commit 83b48de into JedWatson:master Mar 22, 2020
@github-actions github-actions bot mentioned this pull request Mar 22, 2020
@flexdinesh
Copy link
Collaborator

flexdinesh commented Mar 22, 2020

@Tirzono Thanks for the PR. As mentioned in the other comment, we're still ramping up the backlog and you can see more movement in the repo going forward. I've merged this now. I'll add a note here once the patch is released to npm. Thanks again for your patience.

@flexdinesh
Copy link
Collaborator

@Tirzono This fix has been released in version 3.1.0. Thanks again!

https://github.com/JedWatson/react-select/blob/master/packages/react-select/CHANGELOG.md#310

@Tirzono
Copy link
Contributor Author

Tirzono commented Mar 23, 2020

Thank you @flexdinesh, much appreciated!

@andreme
Copy link
Contributor

andreme commented Apr 15, 2020

Not working when using defaultMenuIsOpen or menuIsOpen: https://codesandbox.io/s/zealous-leaf-58ett?file=/src/index.js

It is working when using openMenuOnFocus (and autoFocus).

@Tirzono
Copy link
Contributor Author

Tirzono commented Apr 15, 2020

@andreme You are using an old version in your Codesandbox that doesn't contain this fix.

@andreme
Copy link
Contributor

andreme commented Apr 15, 2020

@Tirzono Sorry, I didn't realise I need to save manually. Updated and showing the issue: https://codesandbox.io/s/zealous-leaf-58ett?file=/src/index.js

@Tirzono
Copy link
Contributor Author

Tirzono commented Apr 15, 2020

I see. Do you mind opening a new issue for this (if there is none already)? I'll have a look if I can find a fix.

@asazernik
Copy link

Bookkeeping note: GH issue for original problem is #3656.

@zzhcch
Copy link

zzhcch commented Apr 24, 2020

@andreme i think the issue in your example is caused by your dataSource which is populating each time along with re-render.
try to move this out of the function or use useMemo would solve the problem.

@andreme
Copy link
Contributor

andreme commented Apr 24, 2020

@zzhcch No, that's not it, I've updated the example
#4013

@zzhcch
Copy link

zzhcch commented Apr 24, 2020

@andreme Ok, I got your point. that seems like an underlying issue.
here is some workaround: #3375

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants