-
Notifications
You must be signed in to change notification settings - Fork 286
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
Implement new design for User Input questions #5890
Comments
@kuasha420 right now there is a possibility that we may remove the open-text input fields for "Other" choices. See the PRD for details. Otherwise, I believe I raised the question on another issue as well but will the questions use a shared component between the dedicated page and the settings page? They're mostly the same are they not? We should avoid duplicate/conflicting efforts between these similar instances as much as we can. Let me know what you think. |
@aaemnnosttv sounds good to me. I have combined #5892 with this one. For the other field, where can I follow the discussion? |
See this thread although there may be another place too. |
A site-specific answer should be preselected if it has already been answered by another user, otherwise a user's own answers would be blank/unselected at this point.
As mentioned above, it sounds like this might change so let's try to word this in a way which doesn't block the issue moving forward here if possible. Apart from these details, LGTM |
Made the necessary changes accordingly. Since Evan is OOO this week, anyone else can have a look and move it to IB if the changes makes sense. Cheers. |
ACs look good here, moving to IB 👍🏻 |
@kuasha420 @tofumatt A note about the following:
Right now, the "Next" button is disabled if the "Other" option is selected and the user does not input something in the textbox. I think from a UX point of view it's much better right now in the sense that the user already knows that he cannot continue unless he enters something in the textbox, which itself has focus when the "Other" option is selected. If we implement the logic as per the designs whereby the user sees an error message, it'll require the user to have an additional click on the "Next" button only to find out he cannot continue to the next page. Let me know what you think. |
QA Update:
|
Absolutely understood and agreed, thank you for the clarification, @aaemnnosttv! I thought the same way, which is why I followed the usage of the tokens defined in the Figma designs and used the relevant SCSS variables. I didn't change the variable values as a part of this issue because I assumed something might be off with the consistency and would need to be addressed separately.
Understood. This could be an inconsistency between the Figma designs (tokens) and our variable values. Please do let me know if you want me to create issues to update these if needed based on the internal discussions. Thank you! |
QA Update:
|
Very interesting findings, thank you @wpdarren!
Hmm, I can't seem to be able to replicate this on my end. Here's a screencast: I think you might be stuck in a scenario where the User Input is required and you can't get past to the Dashboard unless you answer the user input questions. We're removing this as a part of #5900. Can you normally navigate to the Site Kit Dashboard when this happens? I do see the loading animation. Do you think it should be removed when cancelling?
Good spot! I'll open a quick PR to address this.
This isn't a regression from this issue because it happens in the Looking forward to hearing your feedback. Thanks! |
@nfmohit please check against the last release as the I left a review on your PR which looks good but wasn't created from |
@aaemnnosttv My bad. Here's a screenshot from the latest release: However, if you think we should remove the initial focus, I can update that in my PR. CC: @wpdarren
On it now, thank you! |
@nfmohit re your questions.
No, I am not able to navigate to the dashboard. Here's what happens. user-2.mp4
It feels an odd experience to jump to another screen with the loading animation and then load the dashboard. I can create a ticket for this since it probably needs a conversation with Evan/Felix re the implementation here. What do you think? Re. my observation:
I agree this is an issue on the latest version 1.88.0 with the userinput feature flag enabled, but since this is related to the design of the user input screens, do you think we should fix it here, or, should I create a new ticket? |
Thank you for your kind feedback, @wpdarren!
Okay, it looks like what I had thought. You might be stuck in a scenario where the User Input is required and you can't get past to the Dashboard unless you answer the user input questions. We're fixing this as a part of #5900.
I agree. A ticket for this to confirm would be fab, thank you!
Absolutely. I've updated the PR to remove the initial focus. But I'm not 100% sure if it should actually be removed from an accessibility perspective. @aaemnnosttv What do you think? If you think otherwise, I can add it back. Sending it back to you for CR & MR. Thank you folks! |
@wpdarren Based on this comment from @aaemnnosttv, we're not removing the highlight/focus on the first option. We need to create a separate issue to address this. Thank you! |
Follow-up PR has been merged; back to you @wpdarren ! |
QA Update: ✅Verified:
user-input.mp4QA note: there are a few observations that I will create tickets for as per the comments above. |
Feature Description
The question screens of the User Input page should be updated to implement the new Figma design for User Input v2.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Next
button should be keptdisabled
until user selects required minimum number of answer(s).Cancel
button as seen on the design.Implementation Brief
UserInputApp
,assets/js/components/user-input
, as per the designs in Figma on both small and large screens. Notable changes:assets/js/components/user-input/UserInputQuestionWrapper.js
,core/site
datastore via thegetAdminURL
selector withgooglesitekit-dashboard
as parameter.navigateTo
action fromcore/location
for the actual redirection.display: flex
andjustify-content: space-between
on the parent elements of both containers to space out the buttons as per the designs in Figma.assets/js/components/user-input/UserInputPreview.js
since the button should be displayed on all questions and review pages.assets/js/components/user-input/UserInputSelectOptions.js
,onBlur
event) the input text empty after focusing on it.stories/user-input.stories.js
to instead have the existing story live inassets/js/components/user-input/UserInputApp.stories.js
assets/sass/components/user-input/googlesitekit-user-input-questions.scss
and instead display 1 question at a time.Test Coverage
QA Brief
userInput
feature flag is turned on from the tester plugin./wp-admin/admin.php?page=googlesitekit-user-input
URL.Components > User Input
Storybook story.Changelog entry
The text was updated successfully, but these errors were encountered: