feat: refactor UniversalLink component using typescript #6826
Conversation
tomschall
commented
Mar 11, 2025
tomschall
commented
- Refactor the UniversalLink component using typescript.
- Modify lint-staged.config.js to exclude d.ts files
- Modify tsconfig, include types there, otherwise d.ts files will noch be recognized in .tsx files, e.g. inside components
- Use newest version of classnames
✅ Deploy Preview for plone-components canceled.
|
packages/volto/src/components/manage/UniversalLink/UniversalLink.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
| } | ||
|
|
||
| const UniversalLink = React.memo<UniversalLinkProps>( |
There was a problem hiding this comment.
What is the reasoning behind the React.memo?
There was a problem hiding this comment.
It's a performance optimization.
The component gets memoized and if we have for example 50 links in a list and only one changes, with React.memo only the changed link get rendered new.
We can use it here because UniversalLink changes only over props, we have no local states or side effects.
There was a problem hiding this comment.
I made some tests and negative tests to show this optimized behavior and also show when it's not working (unstable props). We always have to use stable references if we want the optimized behavior, that means not passing inline objects to props or inline jsx to children.
I made some detailed examples in the tests, maybe we should also document this.
|
Love the work on converting this very important component to TS, thanks! @sneridagh I will definitely review this over the weekend 👍 |
…ized component behavior using React.memo
* main: (59 commits) Update or remove broken links (#6953) Only accept images in ImageInput component (#6926) Bump vite from 5.4.15 to 5.4.16 (#6932) Fetch vocabularies and querystring config in the current context (#6935) Remove the build from netlify for @plone/components Complete missing german translations and Storybook story for 'ContentsDeleteModal' (#6881) Remove duplicate configuration item `html_title` (#6933) Forwardport hand-cherrypicking the changes happened in Seven for `@plone/registry` relevant for be reintegrated in alpha (`main`). (#6929) Fixed regression on brown bag minor version of `@storybook/addon-webpack5-compiler-babel` (#6930) Amend PR #6885 changelog to be breaking s/Seven/Volto on `main` (#6925) Release @plone/registry 3.0.0-alpha.1 Release @plone/types 2.0.0-alpha.0 Remove news from packages that won't be released from main Fixed merge public folder from addons regression (#6919) Fixed handling of errors that are not coming from the backend (#6903) Move _hardware-requirements.md include from documentation to volto (#6918) Move includes from `plone/documentation` to `plone/volto` (#6917) Fix typo in Appextras documentation (#6083) Fix gray placeholder on horizontal scroll (#6877) ...
|
@pnicolli I rebased this one. Could you please take a final look at it? |
| locale: string; | ||
| }; | ||
| userSession: { | ||
| token: string; |
There was a problem hiding this comment.
| token: string; | |
| token: string | null; |
The default value for token is null. See https://github.com/plone/volto/blob/main/packages/volto/src/reducers/userSession/userSession.js#L14
| (state) => state.userSession?.token, | ||
| ); | ||
|
|
||
| function getUrl(props: UniversalLinkProps, token: string): string { |
There was a problem hiding this comment.
I'm fine with turning the previous code into a function, if we go this way I would extract the function from the component completely so it's also easily testable. This is not mandatory but would be nice imho.
| [email protected]: | ||
| resolution: {integrity: sha512-JR/iSQOSt+LQIWwrwEzJ9uk0xfN3mTVYMwt1Ir5mUcSN6pU+V4zQFFaJsclJbPuAUQH+yfWef6tm7l1quW3C8Q==} |
There was a problem hiding this comment.
I would strongly suggest installing classnames 2.5.1 in @plone/volto-slate as well in order to dedupe the dependency
