-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Improve performance of demo workspace - Rename getImageAbsoluteURIOrBase64
function
#6282
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.
PR Summary
- Renamed
getImageAbsoluteURIOrBase64
togetImageAbsoluteURI
across multiple files. - Removed base64 image handling in
getImageAbsoluteURI
. - Updated avatar URL handling in components like
ParticipantChip.tsx
,CommentHeader.tsx
, andCalendarEventRow.tsx
. - Replaced base64 encoded images with URLs in seed data files (
workspaces.ts
). - Added new tests for
getImageAbsoluteURI
and removed old tests forgetImageAbsoluteURIOrBase64
.
28 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
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.
PR Summary
- Removed base64 image handling in
getImageAbsoluteURI
function across multiple files. - Updated
send-invite-link.email.tsx
to usegetImageAbsoluteURI
. - Modified
CalendarEventRow.tsx
,CommentHeader.tsx
, andParticipantChip.tsx
to usegetImageAbsoluteURI
. - Updated seed data files (
workspaces.ts
) to use URLs instead of base64 images. - Deleted redundant test file
getImageAbsoluteURIOrBase64.test.ts
.
28 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
…rBase64` function
dd8f6d6
to
c51c3a3
Compare
Could you please try to put the calls to this utils only in Avatar-like components, as it's called way too many times and creates an implicit pattern we have to memorize each time we work with avatarUrl instead of just passing whatever URL form we have to the avatar component that will try by itself to create an absoluteURI, because that's its job. |
Hello @lucasbordeau 👋 |
@gitstart-twenty Yes exactly ! We should call this function internally and developers shouldn't think about passing an absolute or relative URL, the Avatar component should take both. |
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.
Hello @lucasbordeau
About this comment we are duplicating the code on twenty-ui
package because we are not sure if we should export the helper function from twenty-ui
or keep the duplicated file as already is done on the email package
Does not seems correct to import the original helper function on Avatar component from twenty-front
because the correct flow would be the twenty-front
package using the twenty-ui
package, so that's why it is duplicated, let us know if this is fine for you.
Log
|
@gitstart-twenty Of course DRY is not an absolute law, it should be put into context, here it's relevant to copy things until we find a good way to handle everything properly, maybe a twenty-shared or twenty-utils package could be interesting ? We'll talk about that with the core team. |
Description
This PR is a continuation of a previous PR: Improve performance of demo workspace #6201 (review)
One test case was removed here:
packages/twenty-front/src/utils/image/__tests__/getImageAbsoluteURI.test.ts
because since we are not handling base64 images anymore, the result is the same of the last test case. Would you rather we update the test instead?Refs
Demo
https://www.loom.com/share/4f32b535c77a4d418e319b095d09452c?sid=df34adf8-b013-44ef-b794-d54846f52d2d
Fixes #3514