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

Add support for the full suite of survey question options in the new survey form #1871

Conversation

henrycatalinismith
Copy link
Collaborator

Creating another side branch off of #1756 in order to make a nice clean space for us to nail down these last little details. There's a bunch of stuff like hidden and required that needs to be handled properly by the new survey form components, and also needs playwright tests to keep an eye on it. To get the ball rolling, I've begun here with the required property on the text questions as it's probably the simplest.

It raises a few little questions.

  1. How do we want to identify required fields to users? We could e.g. add the string (required) on the end of the label. Even just an asterisk on the end is sufficient. (https://www.w3.org/TR/WCAG20-TECHS/H90.html)
  2. How does this change how we structure the playwright tests here? I think the submits responses test has become long enough that its size is beginning to obscure the intent of the individual assertions. So I've kind of sketched out some possible ways of splitting things up more, but haven't invested a lot of love into the ideas there.

Dropping this here already now in a fairly raw initial state just for the benefit of having the progress so far out of my head and onto a real PR. Whatever ideas come to mind from reading this, throw them my way.

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

Nice work! Some comments below. The overall approach looks good to me!

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

I've looked at the code and I think it looks good! I have also tried hiding a survey element and made sure it is not rendered. Marking elements as required is not currently possible in the UI, but the tests seem to verify that it works so that should be fine as soon as we add UI for that.

@henrycatalinismith henrycatalinismith merged commit 8011a09 into issue-1620/next-v13-survey-page-with-app-router-and-server-action Mar 31, 2024
5 checks passed
@henrycatalinismith henrycatalinismith deleted the issue-1620/question-options branch March 31, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants