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

feat: Add 'Other' option for radio/checkbox questions. #1694

Merged
merged 1 commit into from Aug 29, 2023

Conversation

AIlkiv
Copy link
Contributor

@AIlkiv AIlkiv commented Aug 19, 2023

I have implemented a feature to provide other answer for radio buttons and checkboxes.

Related to #93

Demo:

image

image

image

@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #1694 (da261ff) into main (38cae22) will increase coverage by 1.99%.
Report is 12 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1694      +/-   ##
============================================
+ Coverage     40.05%   42.05%   +1.99%     
- Complexity      562      568       +6     
============================================
  Files            55       55              
  Lines          2379     2385       +6     
============================================
+ Hits            953     1003      +50     
+ Misses         1426     1382      -44     

@Chartman123
Copy link
Collaborator

Hi @AIlkiv

thanks for contributing :) I'll have a closer look at it in the next few days. In the meantime you could already go on and fix the errors from our check workflows.

@nextcloud/designers what do you think about the implementation? I'd propose to remove the Other: label in front of the input field and have it has the placeholder inside the field.

@AIlkiv AIlkiv force-pushed the add_field_for_other_answer branch 2 times, most recently from 7f28f36 to 021de39 Compare August 20, 2023 09:20
@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 20, 2023

Fixed

Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a closer look at your changes and left some comments. :-)

lib/Constants.php Outdated Show resolved Hide resolved
src/components/Questions/QuestionMultiple.vue Outdated Show resolved Hide resolved
src/components/Questions/QuestionMultiple.vue Outdated Show resolved Hide resolved
src/components/Questions/QuestionMultiple.vue Outdated Show resolved Hide resolved
src/components/Questions/QuestionMultiple.vue Outdated Show resolved Hide resolved
src/components/Results/ResultsSummary.vue Outdated Show resolved Hide resolved
tests/Unit/Service/SubmissionServiceTest.php Outdated Show resolved Hide resolved
tests/Unit/Controller/ApiControllerTest.php Outdated Show resolved Hide resolved
@marcoambrosini
Copy link
Member

Thanks for the contribution @AIlkiv
@Chartman123 we cannot use placeholders to deliver important information because of accessibility problems.

I think that this text field that is shown when other is selected should appear below the radio button and span the whole width of the container.

Also I think it would be nice to see what these "other" results are in the answers page.

@Chartman123

This comment was marked as resolved.

@marcoambrosini

This comment was marked as resolved.

@AIlkiv

This comment was marked as resolved.

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 23, 2023

@Chartman123 It looks like I fixed everything.

@marcoambrosini
Copy link
Member

@nextcloud/designers what do you think about this? I think it's still best to keep the input field below the radio.
Looks better and works better on mobile screens.

@nimishavijay
Copy link
Member

Very cool! I don't have a strong opinion on the placement of the field, we can show it below like @marcoambrosini said so that it's nicely left aligned with the rest of the options :)

Rest of design looks great, some wording suggestions

  • "People can enter a other answer" --> "People can submit a different answer"
  • "Enter your other answer" --> "Enter your answer" (to avoid duplication of the word "Other")
  • "Other..." --> "Other" (we usually use ellipses when there is some text that is cut off or hidden)

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 23, 2023

Very cool! I don't have a strong opinion on the placement of the field, we can show it below like @marcoambrosini said so that it's nicely left aligned with the rest of the options :)

@nimishavijay

image

image

@Chartman123
Copy link
Collaborator

Two more things:

  • First, I think that we should make sure, that if "other" is selected as answer, the input field should have a value. Currently you can select it but don't have to enter a value. Google allows this, too, but I don't think that this makes sense.
  • I just saw that the hover effect doesn't look good with the text field, for both the in-line version and the new-line version. But I'm not sure what would be the best way to deal with it.

@marcoambrosini If we just add the line break like in the screenshots above, I don't like the look as it moves the text higher than the checkbox next to it. So did you think of a completely new line with the left alignment like for a normal input field? In this case we could also limit the hover effect to just the checkbox and the "Other: " label.

However, I don't think it's too problematic on mobile screens with the in-line field: (compared to the short answer field below)
grafik

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of the positioning of the text field, it shouldn't be within the radio button's boundaries. If we do it inline we need to make sure it wraps on mobile screens

Screenshot 2023-08-24 at 07 35 46

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 24, 2023

Regardless of the positioning of the text field, it shouldn't be within the radio button's boundaries. If we do it inline we need to make sure it wraps on mobile screens

@marcoambrosini In this version, in my opinion, there is no understanding that the radio button and the input field are one field.

In Google Forms on mobile devices, it looks normal when everything is in one row.

@marcoambrosini
Copy link
Member

Google Forms' layout breaks on a mobile screen though.

Screenshot 2023-08-24 at 15 13 11

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 25, 2023

Regardless of the positioning of the text field, it shouldn't be within the radio button's boundaries. If we do it inline we need to make sure it wraps on mobile screens

2023-08-24 на 07 35 46

@marcoambrosini Your example has an additional label, making it appear well-structured. I attempted to break it into two lines, but it seems less intuitive to me (I see four options and a separate option to enter an answer). What are your thoughts?

image

image

@Chartman123
Copy link
Collaborator

@AIlkiv could you try to align the text inside the input field with the text of the other options? And to make it more clear that they both belong together you could change the label to Other: again

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 25, 2023

@AIlkiv could you try to align the text inside the input field with the text of the other options? And to make it more clear that they both belong together you could change the label to Other: again

@Chartman123

image

image

image

@marcoambrosini
Copy link
Member

align the text inside the input field with the text of the other options?

Screenshot 2023-08-25 at 15 44 05

Doing this would break things up visually, and it's not future-proof: If anything changes either in the NcTextfield or in the NcCheckboxRadioSwitch, we'd lose this delicate alignment and it would look off.

Given that:

  • The input is initially disabled
  • There's a label indicating what to do (we can improve the text there)
  • Input becomes active and focused when clicking on "other"
  • There's the label animation when clicking on "other"
    I think it's pretty clear that the input is associated with the "other" option

@AIlkiv I think it's fine if we have the inline option and only wrap it into one above the other below something like 500px.

@Chartman123
Copy link
Collaborator

@marcoambrosini ok, you're right. 👍🏻

@AIlkiv you can use v-if="isMobile" here

@marcoambrosini
Copy link
Member

you can use v-if="isMobile" here

@Chartman123 I don't think we can use isMobile here. isMobile just tells us that the total width of the page is below 1024px, in which the inline input would still look fine.
Conversely, we might have a 4k widescreen but have this form placed in a tiny area of the page or in a dialog.

So what we really care about is the width of the container, but that's a bit hard to "watch". I suggest we put the button and input in a flexbox and give a min-width of 300px to the input with flex-wrap set to wrap

@AIlkiv AIlkiv force-pushed the add_field_for_other_answer branch 2 times, most recently from 26bcad5 to c46ac20 Compare August 26, 2023 05:35
@AIlkiv AIlkiv changed the title Added a new feature: 'Other' option for radio and checkbox questions. feat: Add 'Other' option for radio/checkbox questions. Aug 26, 2023
Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thank you 🎉

Looks good overall, I do not like the otherAnswer name, but should be ok.
So, this good from my point :)

@Chartman123
Copy link
Collaborator

I do not like the otherAnswer name

Do you have something better in mind? We could still change it before merging...

@susnux
Copy link
Collaborator

susnux commented Aug 26, 2023

Do you have something better in mind? We could still change it before merging...

Not really, I just think it is a bit misleading. It sounds like it is possible to have multiple different answers.
Maybe like isRequired we could use allowCustomAnswer?

@Chartman123
Copy link
Collaborator

@AIlkiv Before merging, could you please add the new prop to the docs at docs/API.md and docs/DataStrutcture.md?

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 27, 2023

Maybe like isRequired we could use allowCustomAnswer?

I modified the name to 'allowOtherAnswer' in the 'extraSettings'.

@susnux
Copy link
Collaborator

susnux commented Aug 27, 2023

Maybe like isRequired we could use allowCustomAnswer?

I modified the name to 'allowOtherAnswer' in the 'extraSettings'.

Nice, please push the changes and add it to the api docs, then we can merge this :)

@AIlkiv AIlkiv force-pushed the add_field_for_other_answer branch 2 times, most recently from 8d9e495 to 14ece0a Compare August 27, 2023 13:01
@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 27, 2023

@Chartman123 @susnux Finished

@Chartman123 Chartman123 linked an issue Aug 27, 2023 that may be closed by this pull request
@@ -402,4 +472,24 @@ export default {
}
}
}

.question__other-answer::v-deep {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the deep sector required? If yes we should use :deep() as v-deep id deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for a single selector. Divided into parts with :deep() and without :deep().

@marcoambrosini
Copy link
Member

I had to add a margin after the label "Other:"

@AIlkiv could you add a bigger margin, say 8px?

@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 28, 2023

@AIlkiv could you add a bigger margin, say 8px?

@marcoambrosini Changed code and push

image

@marcoambrosini
Copy link
Member

Sorry @AIlkiv, without the checked feedback the input field looks too far from the label "Other", so could we dial back the margin to 4px. Thank you :)

Signed-off-by: Andrii Ilkiv <a.ilkiv.ye@gmail.com>
@AIlkiv
Copy link
Contributor Author

AIlkiv commented Aug 29, 2023

Sorry @AIlkiv, without the checked feedback the input field looks too far from the label "Other", so could we dial back the margin to 4px. Thank you :)

Ok, refreshed commit.

image

@marcoambrosini
Copy link
Member

Thanks a lot for your contributions @AIlkiv and @Chartman123 ❤️

@marcoambrosini marcoambrosini merged commit 7f0e0da into nextcloud:main Aug 29, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: ❓ question types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

extraSettings not documented Allow custom/other answer for 'multiple' questions (radio & checkbox)
5 participants