Enhancement/9942 siwg woocommerce registration - #13233
Open
zutigrm wants to merge 21 commits into
Open
Conversation
📚 Storybook for d18c9eb: 📦 Build files for d18c9eb:
🎭 Playwright reports for d18c9eb: |
|
Size Change: 0 B Total Size: 3.25 MB ℹ️ View Unchanged
|
tofumatt
requested changes
Aug 5, 2026
tofumatt
left a comment
tofumatt
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, just a few comment tweaks and questions, but overall this seems good-to-go after the few issues mentioned are resolved/answered 👍🏻
Comment on lines
+1008
to
+1009
| * `false` both when WooCommerce is inactive and when it is active but its | ||
| * own account-creation settings are closed. |
Collaborator
There was a problem hiding this comment.
Suggested change
| * `false` both when WooCommerce is inactive and when it is active but its | |
| * own account-creation settings are closed. | |
| * `false` when: | |
| * - WooCommerce is inactive | |
| * - when WooCommerce is active but account-creation in WooCommerce | |
| * is disabled. |
| breakpoint, | ||
| } ) { | ||
| return createInterpolateElement( message, { | ||
| a: showLink ? <Link key="link" href={ settingsURL } /> : <span />, |
Collaborator
There was a problem hiding this comment.
The key prop isn't needed here, from what I can see.
Suggested change
| a: showLink ? <Link key="link" href={ settingsURL } /> : <span />, | |
| a: showLink ? <Link href={ settingsURL } /> : <span />, |
Comment on lines
+63
to
+70
| br: | ||
| breakpoint !== BREAKPOINT_SMALL ? ( | ||
| <br /> | ||
| ) : ( | ||
| // eslint-disable-next-line react/jsx-no-useless-fragment | ||
| <Fragment /> | ||
| ), | ||
| } ); |
Collaborator
There was a problem hiding this comment.
I know this was here before, but why not add a className to this <br /> tag and hide it with CSS instead on small screens? This is sort of odd, and will cause the internal markup of the text to change on screen size changes as well. A pure-CSS implementation would be better.
Comment on lines
46
to
+51
| const anyoneCanRegister = useSelect( ( select ) => | ||
| select( CORE_SITE ).getAnyoneCanRegister() | ||
| ); | ||
| const anyoneCanRegisterWooCommerce = useSelect( ( select ) => | ||
| select( CORE_SITE ).getAnyoneCanRegisterWooCommerce() | ||
| ); |
Collaborator
There was a problem hiding this comment.
Seems like this would make sense to build as a single, consolidated selector. Especially if we want to expand on the value later and not have to manage each setting.
| /** | ||
| * Checks if the registration is open. | ||
| * | ||
| * Checked here in the base class, rather than only in |
Collaborator
There was a problem hiding this comment.
There's redundant phrasing here 🙂
Suggested change
| * Checked here in the base class, rather than only in | |
| * Checked here rather than in |
Comment on lines
+417
to
+421
| * that public static method on the subclass triggers a fatal | ||
| * "Cannot make non static method ... static" error, even though private | ||
| * methods aren't supposed to participate in override compatibility | ||
| * checks. PHP 8.1+ correctly excludes private methods from that check, | ||
| * but the plugin's floor is PHP 7.4. |
Collaborator
There was a problem hiding this comment.
Suggested change
| * that public static method on the subclass triggers a fatal | |
| * "Cannot make non static method ... static" error, even though private | |
| * methods aren't supposed to participate in override compatibility | |
| * checks. PHP 8.1+ correctly excludes private methods from that check, | |
| * but the plugin's floor is PHP 7.4. | |
| * that public static method on the subclass triggers a fatal | |
| * error, even though private methods aren't supposed to | |
| * participate in override compatibility checks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related issue(s):
Allow customers to create an accountsetting #9942Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist