-
Notifications
You must be signed in to change notification settings - Fork 58
feat(ia): settings additional brands #3462
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
Conversation
…s__part-2' into feat/ia-settings-additional-brands
…s__part-2' into feat/ia-settings-additional-brands
added cache get/set helpers
…at/ia-settings-additional-brands
miguelpeixe
left a comment
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.
It's looking great! I've encountered a few issues, which I commented inline.
src/wizards/newspack/views/settings/additional-brands/brand-upsert.tsx
Outdated
Show resolved
Hide resolved
src/wizards/newspack/views/settings/additional-brands/brand-upsert.tsx
Outdated
Show resolved
Hide resolved
src/wizards/newspack/views/settings/additional-brands/brand-upsert.tsx
Outdated
Show resolved
Hide resolved
if `newspack-multibranded-site` plugin is active
to use `defined` vs. `is_plugin_active`
move name from default to placeholder
@miguelpeixe thanks for the thorough review. I've refactored & fixed most of your feedback. Let me thoughts on #3462 (comment) |
miguelpeixe
left a comment
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.
Thank you for all the revisions! It's looking great!
One small adjustment is needed for the new error handling. It's not getting cleared:
After receiving an error due to duplicate slugs, I adjust my slug name and click save. If edit a brand - the same or any other - the error message is still there.
This shouldn't be happening, it should be clearing automatically after a successful subsequent request. I can reproduce, looking into why this is happening, will revert shortly. |
I was mistaken, at one stage this one the plan - at the time it wasn't feasible. Anyway - I have added code that will reset the error on location change 920a27a |
miguelpeixe
left a comment
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.
Thanks for the revisions, Jared! Working great now 💯
All Submissions:
Changes proposed in this Pull Request:
Adds Newspack Multibranded Site wizard to Newspack Plugin Settings Wizard. The majority of functionality is merely a port of the Multibranded plugin.
How to test the changes in this Pull Request:
Other information: