-
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
Re-design the User Input Questionnaire. #9490
Re-design the User Input Questionnaire. #9490
Conversation
Build files for ae09145 have been deleted. |
Size Change: +1.57 kB (+0.09%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
Implemented but I need to update the E2E tests and VRTs. |
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 nice work! I just left few comments
assets/sass/components/user-input/googlesitekit-user-input-module.scss
Outdated
Show resolved
Hide resolved
assets/sass/components/user-input/googlesitekit-user-input-module.scss
Outdated
Show resolved
Hide resolved
assets/sass/components/user-input/googlesitekit-user-input-questions.scss
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, nice one. LGTM
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.
Largely looks good, but there's a few small tweaks/adjustments needed to the text ("etc." as an abbreviation of "et cetera" should have a period at the end).
Summary
Addresses issue:
Relevant technical choices
I implemented based on the updated IB, one point that I skipped was "Remove the now unused assets/js/components/user-input/UserInputPreview.js component." because this component is actually currently used in the settings card when Key Metrics are configured.
There was actually a lot of work here that was not fully captured in the IB including adding new states to the ErrorNotice and ProgressBar as well as updating the E2E tests.
Initially I implemented segmented logic inside of the existing ProgressBar component, however the designs specified different color segments for each step of progress. This would be a real hack with the existing
ProgressBar
component so I made a new cleanProgressSegments
component including storybook 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