-
Notifications
You must be signed in to change notification settings - Fork 27
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
Create a new React hook to access Projects Images rewrite configuration #2639
Conversation
🦋 Changeset detectedLatest commit: 5be06d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit 5be06d1. |
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.
I'm adding some comments to better understand the context of the PR 🙂
...nnectors/src/components/project-extension-image-regex/project-extension-image-regex.spec.tsx
Outdated
Show resolved
Hide resolved
...ll-connectors/src/components/project-extension-image-regex/project-extension-image-regex.tsx
Outdated
Show resolved
Hide resolved
...ll-connectors/src/components/project-extension-image-regex/project-extension-image-regex.tsx
Outdated
Show resolved
Hide resolved
...ll-connectors/src/components/project-extension-image-regex/project-extension-image-regex.tsx
Outdated
Show resolved
Hide resolved
...ll-connectors/src/components/project-extension-image-regex/project-extension-image-regex.tsx
Outdated
Show resolved
Hide resolved
/> | ||
); | ||
const WrappedComponent = (props: Props) => { | ||
const imageregexContext = useProjectExtensionImageRegex(); |
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.
Maybe we can also use a deprecation message with the HOC.
Perhaps something like this:
@commercetools-frontend/application-shell-connectors: It is not recommended to use the 'withProjectExtensionImageRegex' high order component anymore. Please use the 'useProjectExtensionImageRegex' hook instead.
Not 100% sure though since maybe we have customers who still use React class components and they wouldn't be able to use the hook.
Let's see what the rest of the team think about it.
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.
Yes, good point. I am not sure too but it would be nice to see what the team thinks too but I will also update it.
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.
As Carlos mentioned, in some contexts HOC/render prop version is the only choice. On the other hand, for instance Apollo deprecated all their HOCs and render prop components in favour of the hooks in the v3 of their client. For what it's worth, I'm not sure either, but I guess that is OK.
...nnectors/src/components/project-extension-image-regex/project-extension-image-regex.spec.tsx
Outdated
Show resolved
Hide resolved
Thank you, |
I guess the build is failing because now we're using the |
True! Thank you. |
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.
We need to review the version we used for the tiny-warning
message.
Also, I left a couple of comments regarding changeset and docs.
...ll-connectors/src/components/project-extension-image-regex/project-extension-image-regex.tsx
Outdated
Show resolved
Hide resolved
I can raise a new PR for the docs update. |
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.
Good to go from my side 👏
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.
Looking good 👍 I'm leaving only some final (minor) comments.
...nnectors/src/components/project-extension-image-regex/project-extension-image-regex.spec.tsx
Outdated
Show resolved
Hide resolved
...ll-connectors/src/components/project-extension-image-regex/project-extension-image-regex.tsx
Outdated
Show resolved
Hide resolved
/> | ||
); | ||
const WrappedComponent = (props: Props) => { | ||
const imageregexContext = useProjectExtensionImageRegex(); |
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.
As Carlos mentioned, in some contexts HOC/render prop version is the only choice. On the other hand, for instance Apollo deprecated all their HOCs and render prop components in favour of the hooks in the v3 of their client. For what it's worth, I'm not sure either, but I guess that is OK.
9f3c275
to
5be06d1
Compare
Create react hook to access Project's images URLs rewrite rules configurations information from context.
See more on ticket here: #2571