fix(SingleEntitySelectMenuItems): extract `Add New` button from entitiesToSelect #8474

nicolasrouanne · 2024-11-13T10:42:12Z

Description

Extract Add New button from entitiesToSelect and add it as a separate element .
There doesn't seem to be a point in having Add New as part of a list, it seems better off in its own component, apart from list items

Rationale

There already is #8353 addressing the same issue, but it seems it doesn't really remove the duplicate "Add New" in the list, leaving a duplicate "Add New" in SingleEntitySelect

…iesToSelect - and add it as a separate element - to be consistent with `MultiRecordSelect`

nicolasrouanne · 2024-11-13T10:44:22Z

Remaining work:

check if wrapping "Add New" in <DropdownMenuItemsContainer /> is needed, see fix(SingleEntitySelectMenuItems): extract Add New button from entitiesToSelect nicolasrouanne/twenty#1 (comment)
use the same naming convention for MultiSelect and SingleSelect: either Record or Entity see fix(SingleEntitySelectMenuItems): extract Add New button from entitiesToSelect nicolasrouanne/twenty#1 (comment)

…sContainer /> for padding

greptile-apps

PR Summary

This PR extracts the "Add New" button from entity selection lists and places it in its own container for consistent dropdown menu behavior across the application.

Removed onCreate prop and "Add New" button logic from SingleEntitySelectMenuItems.tsx to simplify entity list rendering
Added dedicated CreateNewButton container in SingleEntitySelectMenuItemsWithSearch.tsx with proper separators
Standardized dropdown menu behavior by ensuring "Add New" button appears in its own section below entity list
Fixed duplicate "Add New" button issue present in previous PR fixes: Standardize "Add New" button in dropdown menus #8169 #8353

_{2 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

...y-front/src/modules/object-record/relation-picker/components/SingleEntitySelectMenuItems.tsx

...c/modules/object-record/relation-picker/components/SingleEntitySelectMenuItemsWithSearch.tsx

…e of an "*end" positioned `dropdownPlacement` - display is inverted when the `Placement` is at the `end`, following the `MultiRecordSelect` component design - add a story to display the `end` placement - pass down the `dropdownPlacement` prop in `SingleEntitySelect`

nicolasrouanne

I passed down a prop in SingleEntitySelect. Don't know if it was a good idea, I can remove it if needed.

nicolasrouanne · 2024-11-13T14:16:04Z

...ges/twenty-front/src/modules/object-record/relation-picker/components/SingleEntitySelect.tsx

@@ -15,6 +15,7 @@ export type SingleEntitySelectProps = {

 export const SingleEntitySelect = ({
  disableBackgroundBlur = false,
+  dropdownPlacement,


I took the liberty of adding this prop and passing it down to SingleEntitySelectMenuItemsWithSearch. I don't know if it's a good idea:

it does expand SingleEntitySelect contract... which I recon can be less maintainable.

however the dropdownPlacement is already a prop available in SingleEntitySelect type interface; it just wasn't passed down to the underlying component, which I found was weird

it allows testing and using this prop in Storybook

Let me know what you think.

Yes makes sense to pass it if the below component has it I agree!

nicolasrouanne · 2024-11-13T14:18:06Z

.../modules/object-record/relation-picker/components/__stories__/SingleEntitySelect.stories.tsx

+export const WithEndPlacement: Story = {
+  args: { dropdownPlacement: 'bottom-end' },
+};


I added a story to check the select with an end placement

FelixMalfait · 2024-11-13T18:19:26Z

This looks great thanks a lot!

This introduced a small bug I think, I haven't dug into it yet
https://github.com/user-attachments/assets/c9ef43d5-3b51-4a57-8dcd-0bb056a49481

FelixMalfait · 2024-11-13T18:23:26Z

Actually I think there's a misunderstanding on what placement is supposed to be. This was a hack to maintain a stable place input. So the text input always remained in the same position, and the results were displayed either below or after the input depending on the space available.
Now we have a weird behavior, where -for example- if you have 4 results it's displayed on top and then if you type one character and there are now two results then your text input moves down by 40px because results are now placed at the bottom.
Ideally we could even have frozen completely the placement once a dropdown is open (along with chosing the right input positioning based on the initial space available / recommended placement) but this was too complex so unless it appears as a something that would simplify the task for you, it's not a requirement

nicolasrouanne · 2024-11-14T10:05:31Z

Thanks for your explanation, I would never have that figured out. I'm gonna try to dig a little deeper, just out of curiosity, to understand why the dropdown is out there floating. I'm gonna try to run the playwright tests too.

Once that's done, I'll remove exposing the dropdownPlacement and passing it down, which I think is the reason for the regression.

FelixMalfait · 2024-11-14T10:07:35Z

@nicolasrouanne thanks a lot! Let us know if it's too much word and you want us to take over

…`SingleEntitySelect` - and passing it down to `SingleEntitySelectMenuItemsWithSearch` - since it broke the position of the dropdown, see twentyhq#8474 (comment)

nicolasrouanne · 2024-11-14T10:54:00Z

I reverted exposing the dropdownPlacement prop in SingleEntitySelect, see c255ae5, it should fix the dropdown placement regression

I tried to reproduce the bug using E2E tests, FWIW I didn't really managed to get E2E tests running in my local environment. Here's what I did:

cp ./packages/twenty-e2e-testing/.env.example ./packages/twenty-e2e-testing/.env # setup E2E env variables
npx nx test:e2e:setup twenty-e2e-testing # install playwright
npx nx database:reset twenty-server # reset dev database to start with a clean slate
npx nx start # start local dev server
npx nx test:e2e:ui twenty-e2e-testing # run using --ui to see playwright ui and browser

Tests failed in login and all subsequent tests using a login-like behaviour since I never get the "Continue with email" button, I always have the [email protected] prefilled, with a "Continue" button.

It's probably an environment misconfiguration, I stopped investigating for now.

Note

I feel that the developer guide would benefit from an explanation on how to run E2E tests

FelixMalfait

Great work! Thanks a lot. I didn't realize this would be so complex to fix everything, there are still a few glitches unrelated to the initial ask which we'll fix in another PR.

Agree with E2E tests, it's such a pain that I've never ran them locally I just wait for the CI

github-actions · 2024-11-16T05:50:49Z

Thanks @nicolasrouanne for your contribution!
This marks your 2nd PR on the repo. You're top 20% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

FelixMalfait · 2024-11-17T08:58:26Z

...c/modules/object-record/relation-picker/components/SingleEntitySelectMenuItemsWithSearch.tsx

-
-  let onCreateWithInput = undefined;
-
-  if (isDefined(onCreate)) {


FYI I think we should have kept that #8529 (comment)

Sorry to have introduced that 😢
I think I didn't really understand what it was used for.
I was under the impression my change didn't impact the public API of the component... but it seems I was wrong, sorry.

It's weird that TS didn't catch it

No problem at al! I should have seen it 😅 (+ lack of good tests on our codebase). That code looks ugly I would have been tempted to remove it too!

fix(SingleEntitySelectMenuItems): extract Add New button from entit…

58cb1f7

…iesToSelect - and add it as a separate element - to be consistent with `MultiRecordSelect`

fix(SingleEntitySelectMenuItems): wrap "Add New" in <DropdownMenuItem…

9421b02

…sContainer /> for padding

nicolasrouanne marked this pull request as ready for review November 13, 2024 11:10

greptile-apps bot reviewed Nov 13, 2024

View reviewed changes

...y-front/src/modules/object-record/relation-picker/components/SingleEntitySelectMenuItems.tsx Outdated Show resolved Hide resolved

...c/modules/object-record/relation-picker/components/SingleEntitySelectMenuItemsWithSearch.tsx Outdated Show resolved Hide resolved

nicolasrouanne commented Nov 13, 2024

View reviewed changes

...c/modules/object-record/relation-picker/components/SingleEntitySelectMenuItemsWithSearch.tsx Outdated Show resolved Hide resolved

...c/modules/object-record/relation-picker/components/SingleEntitySelectMenuItemsWithSearch.tsx Show resolved Hide resolved

nicolasrouanne added 2 commits November 13, 2024 14:04

fix(SingleEntitySelectMenuItems): remove deleted prop from type

d22bbc3

nicolasrouanne commented Nov 13, 2024

View reviewed changes

nicolasrouanne requested a review from FelixMalfait November 13, 2024 14:20

fix(SingleEntitySelect): revert exposing dropdownPlacement prop in …

c255ae5

…`SingleEntitySelect` - and passing it down to `SingleEntitySelectMenuItemsWithSearch` - since it broke the position of the dropdown, see twentyhq#8474 (comment)

FelixMalfait added 3 commits November 15, 2024 17:51

Don't shop separator if no create

b66fa5a

Merge main

a09cd3c

Small ui adjustments

b7ece62

FelixMalfait approved these changes Nov 16, 2024

View reviewed changes

FelixMalfait merged commit dc42315 into twentyhq:main Nov 16, 2024
17 checks passed

FelixMalfait reviewed Nov 17, 2024

View reviewed changes

bosiraphael mentioned this pull request Nov 18, 2024

fixes: Standardize "Add New" button in dropdown menus #8169 #8353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(SingleEntitySelectMenuItems): extract `Add New` button from entitiesToSelect #8474

fix(SingleEntitySelectMenuItems): extract `Add New` button from entitiesToSelect #8474

nicolasrouanne commented Nov 13, 2024

nicolasrouanne commented Nov 13, 2024 •

edited

Loading

greptile-apps bot left a comment

nicolasrouanne left a comment

nicolasrouanne Nov 13, 2024

FelixMalfait Nov 13, 2024

nicolasrouanne Nov 13, 2024

FelixMalfait commented Nov 13, 2024

FelixMalfait commented Nov 13, 2024 •

edited

Loading

nicolasrouanne commented Nov 14, 2024

FelixMalfait commented Nov 14, 2024

nicolasrouanne commented Nov 14, 2024

FelixMalfait left a comment

github-actions bot commented Nov 16, 2024

FelixMalfait Nov 17, 2024

nicolasrouanne Nov 17, 2024

FelixMalfait Nov 17, 2024


		let onCreateWithInput = undefined;

		if (isDefined(onCreate)) {

fix(SingleEntitySelectMenuItems): extract Add New button from entitiesToSelect #8474

fix(SingleEntitySelectMenuItems): extract Add New button from entitiesToSelect #8474

Conversation

nicolasrouanne commented Nov 13, 2024

Description

Rationale

nicolasrouanne commented Nov 13, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

nicolasrouanne left a comment

Choose a reason for hiding this comment

nicolasrouanne Nov 13, 2024

Choose a reason for hiding this comment

FelixMalfait Nov 13, 2024

Choose a reason for hiding this comment

nicolasrouanne Nov 13, 2024

Choose a reason for hiding this comment

FelixMalfait commented Nov 13, 2024

FelixMalfait commented Nov 13, 2024 • edited Loading

nicolasrouanne commented Nov 14, 2024

FelixMalfait commented Nov 14, 2024

nicolasrouanne commented Nov 14, 2024

FelixMalfait left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 16, 2024

FelixMalfait Nov 17, 2024

Choose a reason for hiding this comment

nicolasrouanne Nov 17, 2024

Choose a reason for hiding this comment

FelixMalfait Nov 17, 2024

Choose a reason for hiding this comment

fix(SingleEntitySelectMenuItems): extract `Add New` button from entitiesToSelect #8474

fix(SingleEntitySelectMenuItems): extract `Add New` button from entitiesToSelect #8474

nicolasrouanne commented Nov 13, 2024 •

edited

Loading

FelixMalfait commented Nov 13, 2024 •

edited

Loading