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

feat: display record identifier field as first column in table #3788

Merged
merged 11 commits into from
Feb 9, 2024

Conversation

thaisguigon
Copy link
Contributor

& forbid hiding and moving record identifier column

Closes #3303

@thaisguigon thaisguigon force-pushed the feat/label-identifier-first-table-column branch from bc798db to c7b3cbd Compare February 2, 2024 17:35
@thaisguigon thaisguigon marked this pull request as ready for review February 2, 2024 17:37
Copy link

github-actions bot commented Feb 2, 2024

TODOs/FIXMEs:

  • // Todo replace this with label identifier logic: packages/twenty-front/src/modules/object-record/record-index/options/hooks/useRecordIndexOptionsForBoard.ts

Generated by 🚫 dangerJS against 761658c

@lucasbordeau
Copy link
Contributor

Should we move the "+" new button on the record identifier row ?

image

@lucasbordeau
Copy link
Contributor

Should we move auto-complete "Untitled" on the record identifier field ?

image

@lucasbordeau
Copy link
Contributor

Is it relevant to fix this in this PR ?

The breadcrumb should show the record identifier content :

Capture d’écran 2024-02-06 à 16 03 03


const availableColumnKeys = get(
availableTableColumnsStateScopeMap({ scopeId }),
).map(({ fieldMetadataId }) => fieldMetadataId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we use a dedicated selector for that ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@thaisguigon
Copy link
Contributor Author

Should we move the "+" new button on the record identifier row ?

Good catch, done!

@thaisguigon
Copy link
Contributor Author

Is it relevant to fix this in this PR ?

The breadcrumb should show the record identifier content

Absolutely! Done, thank you!

@thaisguigon
Copy link
Contributor Author

Should we move auto-complete "Untitled" on the record identifier field ?

Not sure how to go about this one, as the "Untitled" value is generated on backend (default value for the "name" field). Maybe @Weiko has some input on this?

@Weiko
Copy link
Member

Weiko commented Feb 6, 2024

Should we move auto-complete "Untitled" on the record identifier field ?

Not sure how to go about this one, as the "Untitled" value is generated on backend (default value for the "name" field). Maybe @Weiko has some input on this?

That means "default value" should be editable for existing fields. I'm not sure this has been really tested since the UI does not allow that, but yes it should work 🤔.
We could also make this field required, by also editing the isNullable. However I'm pretty sure that this case is not managed by our migration service since updating nullability on DB level has some edge cases to handle (for example if there is existing rows). => Actually we already create TEXT field as non-nullable (cc @charlesBochet).
I don't have a clear answer yet, I suggest that we discuss with the product as well 👍

@charlesBochet
Copy link
Member

@Weiko @thaisguigon
From a DB perspective, it is non nullable because all text fields are non nullable have a default value: "".

From an API perspective, all fields are non required because they are either nullable in DB or have a default value. (here name has default value so is non-required)

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

@thaisguigon I think we are adding too much complexity on the recordTable:

  • I think we should aim to reduce the number of selectors we have as this gets quickly out of control : contributors are not forced to re-use selectors so they usually re-code the logic again and again, or introduce their own selectors that are slightly different. It's hard to get this factored. I prefer having basic states and basic selector and factorize logic in components or utils if it's needed, but usually it is not: for example the availableTableColumnKey is only extracting the availableTableColumnKey
  • I think we should not add all this logic about labelIdentifiers fetching in the table itself. We could add it to mapViewFieldsToColumnDefinitions "isLabelIdentifer"?

@thaisguigon thaisguigon force-pushed the feat/label-identifier-first-table-column branch 2 times, most recently from b824666 to 6c24b79 Compare February 7, 2024 16:50
@thaisguigon
Copy link
Contributor Author

@thaisguigon I think we are adding too much complexity on the recordTable:

  • I think we should aim to reduce the number of selectors we have as this gets quickly out of control : contributors are not forced to re-use selectors so they usually re-code the logic again and again, or introduce their own selectors that are slightly different. It's hard to get this factored. I prefer having basic states and basic selector and factorize logic in components or utils if it's needed, but usually it is not: for example the availableTableColumnKey is only extracting the availableTableColumnKey
  • I think we should not add all this logic about labelIdentifiers fetching in the table itself. We could add it to mapViewFieldsToColumnDefinitions "isLabelIdentifer"?

@charlesBochet Done:

  • removed some selectors
  • added some utils
  • moved the label identifier logic to mapViewFieldsToColumnDefinitions

@thaisguigon thaisguigon force-pushed the feat/label-identifier-first-table-column branch 2 times, most recently from ffa2df1 to 83e9b8f Compare February 8, 2024 10:18
@thaisguigon thaisguigon marked this pull request as draft February 8, 2024 11:17
@thaisguigon thaisguigon force-pushed the feat/label-identifier-first-table-column branch from eaf9dbd to 761658c Compare February 9, 2024 10:30
@thaisguigon thaisguigon marked this pull request as ready for review February 9, 2024 10:30
@charlesBochet charlesBochet merged commit 201a2c8 into main Feb 9, 2024
14 checks passed
@charlesBochet charlesBochet deleted the feat/label-identifier-first-table-column branch February 9, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Timebox] RecordTables - Display Record identifier field as first column + forbid moving and hiding
4 participants