-
Notifications
You must be signed in to change notification settings - Fork 295
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
Finish refactoring legacy stories. #9941
Finish refactoring legacy stories. #9941
Conversation
e79a263
to
cb9cf23
Compare
cb9cf23
to
366582b
Compare
…infrastructure/9882-complete-stories-refactor.
Awaiting the dependent branch to be merged. |
Build files for 51c3583 have been deleted. |
Size Change: 0 B Total Size: 1.98 MB ℹ️ View Unchanged
|
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.
Hi @benbowler, great work on this migration, it's quite a task.
I've identified a number of scenarios that we should fix and retain in some form. I've also left a handful of other comments. Please take a look.
Also noting that I've again been doing QA:Eng testing as part of the CR process, so once you've addressed the review comments I can review the new/affected stories and wrap up the QA as well.
assets/js/modules/adsense/components/settings/SettingsForm.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/adsense/components/settings/SettingsView.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/adsense/components/settings/SettingsView.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/adsense/components/settings/SettingsForm.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/tagmanager/components/settings/SettingsView.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/tagmanager/components/settings/SettingsForm.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/tagmanager/components/common/AMPContainerSelect.stories.js
Show resolved
Hide resolved
assets/js/modules/tagmanager/components/common/WebContainerSelect.stories.js
Show resolved
Hide resolved
Thanks @techanvil, lots to review here too! I have addressed your comments. As an additional change I moved the Search Console module settings components to match all other modules. |
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 @benbowler! This is looking great. Just a couple more comments from me to address and this should be good to go.
assets/js/modules/tagmanager/components/settings/SettingsForm.stories.js
Outdated
Show resolved
Hide resolved
assets/js/modules/tagmanager/components/settings/SettingsForm.stories.js
Outdated
Show resolved
Hide resolved
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 @benbowler!
Looks like you missed this comment on my previous review though, #9941 (comment)
Once that is addressed this will be good to go.
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 @benbowler, that's great!
Please note, I pushed a commit to address the last stories referenced in this comment which needed an update to ensure the snippet toggle was displayed.
I've confirmed the failing JS and E2E tests are unrelated to the changes in this PR.
Top work, LGTM, and good to go!
✅
Summary
Addresses issue:
Relevant technical choices
Completed the Storybook refactor of legacy stories!
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