Skip to content
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

Remove text field from the "Other" option in all User Input Questions #6181

Closed
kuasha420 opened this issue Nov 21, 2022 · 8 comments
Closed
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@kuasha420
Copy link
Contributor

kuasha420 commented Nov 21, 2022

Feature Description

All the user input question has an 'Other' options which allows users to input their own answers in a free form text field. This UI/UX should be removed from both User Input (including inline Edit) screen and the Settings (Edit) screens and replaced with a simple "Other" option without the text field.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The "Other" option in all User Input questions should be turned into regular option without the text field.
  • This should happen across all User Input screens. ie. questions, reviews, inline edit and settings edit.
  • This also means removing any logic tied to the text field and cleaning up unnecessary logic and code after the removal of the aforementioned text field.
  • No migration is necessary for users who already answered "Other" with a text answer since the feature has not launched yet.
    • However, make sure it doesn't introduce any UI or console error when an user has already answered other, just silently ignoring the answer and treating the question as unanswered should be enough.

Implementation Brief

  • In assets/js/components/user-input/util/constants.js:
    • In the object returned from getUserInputAnswers() function, add a new property having other as the key and Other (translatable) as the value at the end of each nested object (answer item).
  • In assets/js/components/user-input/UserInputSelectOptions.js:
    • Inside the return statement, remove the div.googlesitekit-user-input__select-option containing the Other <ListComponent />.
    • Remove the following states, their usages, setters, and any logic around them:
      • other
      • hasOtherError
      • disabled
    • Remove inputRef and its usages.
    • Remove the following functions:
      • onOtherChange
      • onOtherBlur
  • In assets/js/components/user-input/UserInputPreviewGroup.js:
    • Remove the definition and usage of the sprintfTemplate variable.
  • In assets/sass/components/user-input/googlesitekit-user-input-controls.scss:
    • Remove all styles for .googlesitekit-user-input__select-option .mdc-text-field.
    • Remove all styles for .googlesitekit-user-input__select-option-text-field.

Test Coverage

  • No tests need to be added/updated.
  • Fix any failing tests.

QA Brief

  • Ensure that the userInput feature flag is enabled in the tester plugin.
  • Go to the User Input screen.
  • Verify that all the questions have the Other option at the end has no text field.
  • Verify that using the Other option without the text field works as expected all along.
  • Verify that there are no console errors.

Changelog entry

  • Remove text field from the "Other" option in all User Input questions.
@tofumatt
Copy link
Collaborator

ACs look good, thanks! 👍🏻

@tofumatt tofumatt removed their assignment Nov 23, 2022
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Nov 24, 2022
@FlicHollis
Copy link
Collaborator

Hi @nfmohit please could you add a priority to this ticket. Thanks!

@tofumatt tofumatt added the P0 High priority label Nov 24, 2022
@tofumatt tofumatt self-assigned this Nov 24, 2022
@tofumatt
Copy link
Collaborator

Update the options prop by adding a new property having other as the key and Other (translatable) as the value.

You mention updating the options prop in assets/js/components/user-input/UserInputSelectOptions.js, but that prop isn't declared with defaults in that file. The user input code is admittedly a bit hard-to-follow for me, but I'm not sure where this new property will be added based on the IB 🤔

A similar question for the changes to options mentioned in assets/js/components/user-input/UserInputPreview.js—could you be more specific with the change? I don't see what's really supposed to change as that file renders several components with an options prop, though they're all UserInputPreviewGroup

@tofumatt tofumatt assigned nfmohit and unassigned tofumatt Nov 25, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Nov 25, 2022

Thank you for the kind review and for adding the priority on my behalf, @tofumatt!

You mention updating the options prop in assets/js/components/user-input/UserInputSelectOptions.js, but that prop isn't declared with defaults in that file. The user input code is admittedly a bit hard-to-follow for me, but I'm not sure where this new property will be added based on the IB 🤔

I was thinking of something like this just at the start of the function:
options[ 'other' ] = __( 'Other', 'google-site-kit' );

But you're right, since the options prop doesn't have a default, it's not likely but could cause side effects. How about we set an empty object as a default for options since it expects an object anyway?

A similar question for the changes to options mentioned in assets/js/components/user-input/UserInputPreview.js—could you be more specific with the change? I don't see what's really supposed to change as that file renders several components with an options prop, though they're all UserInputPreviewGroup…

My mistake here. The file should've been assets/js/components/user-input/UserInputPreviewGroup.js.

I have updated the IB with the above findings. Let me know what you think, thanks!

@nfmohit nfmohit assigned tofumatt and unassigned nfmohit Nov 25, 2022
@tofumatt
Copy link
Collaborator

Ohhh, I see what you're proposing now. Basically adding the "Other" option to the supplied options passed to the component.

Given there's only three options, I think it'd be better to add the "Other" option to each object here:

export function getUserInputAnswers() {

It's less "DRY", but it also means we won't have to re-architect the questions if we ever want a question without an "Other" value, and it's also less complexity/code to write. Let's do it that way rather than trying to automate it 🙂

@tofumatt tofumatt assigned nfmohit and unassigned tofumatt Nov 28, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Nov 29, 2022

Makes sense, I've updated the IB. Thank you @tofumatt!

@tofumatt
Copy link
Collaborator

Thanks @nfmohit, I think that's a lot more straightforward 🙂

IB ✅

@tofumatt tofumatt removed their assignment Nov 30, 2022
@nfmohit nfmohit self-assigned this Dec 6, 2022
@nfmohit nfmohit removed their assignment Dec 6, 2022
@techanvil techanvil assigned techanvil and nfmohit and unassigned techanvil Dec 6, 2022
techanvil added a commit that referenced this issue Dec 6, 2022
…ext-field

[User Input] Remove text field from `Other` options
@mohitwp mohitwp self-assigned this Dec 6, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Dec 7, 2022

QA Update ✅

  • Verified on dev.
  • Verified questions, reviews, inline edit and settings edit.
  • Text field removed successfully from the 'Other' Option.
  • No found any console errors.

image

image

image

image

@mohitwp mohitwp removed their assignment Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants