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

fix glitch for relation picker search #8040

Merged
merged 9 commits into from
Oct 25, 2024

Conversation

sans-byte
Copy link
Contributor

Fix for #7957

@FelixMalfait FelixMalfait self-assigned this Oct 25, 2024
Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

LGTM, I still have an issue with kanba relation picker though (not loading)

placement: dropdownPlacement,
middleware: [flip(), ...offsetMiddlewares],
whileElementsMounted: autoUpdate,
strategy: dropdownStrategy,
});

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how we can get rid of this useEffect 🤔

@FelixMalfait FelixMalfait merged commit 9c923ba into twentyhq:main Oct 25, 2024
16 checks passed
hasMaxHeight?: boolean;
}) => {
return (
<StyledDropdownMenuItemsExternalContainer
hasMaxHeight={hasMaxHeight}
hasMinHeight={hasMinHeight}
Copy link
Contributor

Choose a reason for hiding this comment

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

@FelixMalfait Was this causing any issues? Because this was fix for #7959 With the Removal of this, I can see this issue happening again.

Copy link
Member

Choose a reason for hiding this comment

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

@harshrajeevsingh yes sorry I realized while reviewing this PR that this wasn't the right fix for #7959. It's not very pretty to have a constant height and that was not the desired outcome. You can see in this PR how I introduced dropdown placement instead to keep the input fixed but still have it be positionned properly (to the top or toward the bottom) depending on the space available

Copy link
Member

Choose a reason for hiding this comment

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

Note what I did was improve the design based on placement, but the placement still changes, it's less of an issue because the focus area (input) stay in the same place. But if we wanted to go further we could have forced the placement to stay still also in this situation (but without min height, through the tools provided by the useFloatting hook)

Copy link
Contributor

Choose a reason for hiding this comment

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

@FelixMalfait Ok got it. Thanks for the input. : )
Also, I found another issue. While searching, the "No result" flashes. Have a look:
Screencast from 2024-10-28 12-41-10.webm

Do you think this issue should be raised?

Copy link
Member

Choose a reason for hiding this comment

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

@harshrajeevsingh it would be great to fix it yes! Thanks!

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.

4 participants