-
Notifications
You must be signed in to change notification settings - Fork 8
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(tp/con): misc fixes #674
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@katamatata, good work here. I know how much testing was required to make this happen.
A simple pattern strikes me: whenever the menuPosition
prop is set, the menuPortalTarget
prop is set to document.body
. How about we:
- remove the
menuPortalTarget
as a prop - narrow down
menuPosition
to the same values accepted by<Select menuPosition={}>
(see this for how to access the correct type) - inside of our
<FormSelect>
component, conditionally setmenuPortalTarget
todocument.body
ifBoolean(menuPosition)
I'm also curious to see what @helloanil thinks of this PR. |
The PR looks good to me except the thing @ericbolikowski mentioned. What @ericbolikowski mentions definitely makes sense and cleans up the clutter however as far as I see, the solution in this PR is not really working. We simply fix it for some Selects, but not for others. See here: CleanShot.2023-03-12.at.20.39.23.mp4I really liked the idea that the Selects at the upper section of the Modal are more likely to be impacted by scrolling in the modal so it's smart to tackle them but not the ones at the bottom. However, depending on the screen size and resolution of the user, the Selects we decide to fix or not here might still impact the UX. I gave it a little look and found this issue. So apparently we were not the only ones who are impacted. There are several solutions recommended and liked by others, in the long issue thread. But the real solution that fixed the issue seemed to be bumping up the react-select version to v5.5.0 or later. We are currently using v3 which is 2 versions below the fixed version. This means there are some breaking changes and we need to be careful that everything works, if we go ahead and update. There are two things need to be done:
@katamatata @ericbolikowski Sorry for making this comment this long. I definitely don't want this PR to stay around too long and become a pain in the butt for anyone. I'm happy to approve it with the slight improvement that Eric suggests. However then I would love to see us working on the react-select v5 migration to put the real nail in the coffin 🔨 ⚰️ Let me know what you think! |
Thanks for your comment and thorough testing, @helloanil! |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
What Github issue does this PR relate to? Insert link.
#387
What should the reviewer know?
Done:
FormSelect
input field and added typed props.Screenshots
Screen.Recording.2023-03-13.at.22.22.30.mov