feat: add table columns #1056
feat: add table columns #1056
Conversation
|
Hi 👋 Here's a preview environment 🚀 https://front-twentyhq-twenty-1056.env.ergomake.link Environment Summary 📑
Here are your environment's logs. For questions or comments, join Discord. Click here to disable Ergomake. |
thaisguigon
force-pushed
the
feat/add-view-field
branch
3 times, most recently
from
b97059d
to
4848b61
Compare
There was a problem hiding this comment.
Looks good to me. Unfortunately they are conflicts with the branch I've merged this morning: #1061
In the idea, I've done a minor change to remove props drilling on EntityTableHeader as we are using recoil state anyway. hope it's not too painful to resolve! LGTM!
Closes #879
thaisguigon
force-pushed
the
feat/add-view-field
branch
from
4848b61
to
2b56ebd
Compare
|
@thaisguigon any hints to test the feature ? I can not see the (+) button when trying locally: |
| children: React.ReactElement[]; | ||
| }; | ||
| children: React.ReactElement | React.ReactElement[]; | ||
| } & Omit<ComponentProps<'div'>, 'children'>; |
There was a problem hiding this comment.
can we do it the other way?
type IconButtonGroupProps = ComponentProps<'div'> & {
variant: IconButtonVariant;
size: IconButtonSize;
children: React.ReactElement | React.ReactElement[];
};
There was a problem hiding this comment.
Done. I kept the Omit<..., 'children'> though, otherwise IconButtonGroupProps['children']'s type would then become ReactNode & (React.ReactElement | React.ReactElement[]).
| } | ||
| `; | ||
|
|
||
| type EntityTableColumnMenuProps = { |
There was a problem hiding this comment.
we usually use type OwnProps in the codebase, I don't know what code convention we should choose. The one you use looks more flexible if we have multiple components in the same file
There was a problem hiding this comment.
I usually prefix the component name to the type name in case we need to export the Props type to reuse it later (in stories, in a parent component...). But if the Props are kept locally then it could be OwnProps. Would you rather I change it as OwnProps here?
There was a problem hiding this comment.
I actually like your convention more, it looks more flexible. The only drawback is that when we rename a component, we have one more renaming to do but with IDEs capabilities this should not be a problem. Let's leave it like this and we will progressively replace the OwnProps. I will also add it to the doc
| setIsColumnMenuOpen((previousValue) => !previousValue); | ||
| }, []); | ||
|
|
||
| const handleAddViewField = useCallback( |
There was a problem hiding this comment.
LGTM, but I feel we will need to extract that to a hook in the next PR as we will be able to add viewFields from two different places (Options menu and + icon in the table header)
| } from '../types/ViewField'; | ||
|
|
||
| export const viewFieldsState = atom<{ | ||
| objectName: 'company' | 'person' | ''; |
There was a problem hiding this comment.
I'm surprised about the '' possibility? Can we get rid of it?
There was a problem hiding this comment.
I added an empty possibility because I was not sure what value to use by default: company or person? Not sure if it makes sense to choose one when no viewFields have been fetched yet.
There was a problem hiding this comment.
Are forced to pass a default? Maybe it doesn't make sense for viewFields to have a default and it should always be set with something that makes sense
To test it I delete a |
thaisguigon
added
run-chromatic
-PR: awaiting review
and removed
-PR: awaiting author
labels
Closes #879