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

PC-505: Add new GBIS feedback questions #239

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

chidin194
Copy link
Contributor

@chidin194 chidin194 commented Oct 20, 2023

New GBIS feedback form, pending Welsh translations from DESNZ

@chidin194
Copy link
Contributor Author

From a design perspective, is this error enough? I was also considering adding a red margin to each question, but that might be confusing since none are specifically mandatory:

image

@chidin194 chidin194 force-pushed the pc-505-new-feedback-survey branch from b8dee82 to b861425 Compare October 20, 2023 16:11
Copy link
Contributor

@AyaPK AyaPK left a comment

Choose a reason for hiding this comment

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

A few things to look at but mostly looks good!

help_to_heat/templates/frontdoor/feedback.html Outdated Show resolved Hide resolved
@AyaPK
Copy link
Contributor

AyaPK commented Oct 23, 2023

Regarding the error message

From a design perspective, is this error enough? I was also considering adding a red margin to each question, but that might be confusing since none are specifically mandatory:

image

I'd say to look at the component style guidelines, and actually use an error summary component for this
https://design-system.service.gov.uk/components/error-summary/

@chidin194
Copy link
Contributor Author

chidin194 commented Oct 23, 2023

Regarding the error message

From a design perspective, is this error enough? I was also considering adding a red margin to each question, but that might be confusing since none are specifically mandatory:
image

I'd say to look at the component style guidelines, and actually use an error summary component for this https://design-system.service.gov.uk/components/error-summary/

So I've used the error summary class, but this way of validating a form doesn't fit in with the usual validation pattern on the design system as we're not validating any one specific field in particular. Also, adding a separate error message alongside each field might require more extensive changes since we get the data in a blob. Maybe this needs a separate conversation with the client! @AyaPK @CheshireSwift

@chidin194 chidin194 force-pushed the pc-505-new-feedback-survey branch from b861425 to d63cc3e Compare October 23, 2023 18:19
@AyaPK
Copy link
Contributor

AyaPK commented Oct 24, 2023

So I've used the error summary class, but this way of validating a form doesn't fit in with the usual validation pattern on the design system as we're not validating any one specific field in particular. Also, adding a separate error message alongside each field might require more extensive changes since we get the data in a blob. Maybe this needs a separate conversation with the client! @AyaPK @CheshireSwift

I'd posit you could just do something like:

<div class="govuk-error-summary" data-module="govuk-error-summary">
  <div role="alert">
    <h2 class="govuk-error-summary__title">
      There was a problem submitting the form
    </h2>
    <div class="govuk-error-summary__body">
      <ul class="govuk-list govuk-error-summary__list">
        <li>
          <a href="#">Please answer at least one question before submitting feedback</a>
        </li>
      </ul>
    </div>
  </div>
</div>

which would look much nicer, fitting the style guidelines and using the component as intended

@chidin194
Copy link
Contributor Author

New and improved error screen:
image

@chidin194 chidin194 force-pushed the pc-505-new-feedback-survey branch from d63cc3e to 86ffa09 Compare October 24, 2023 16:27
Copy link
Contributor

@CheshireSwift CheshireSwift left a comment

Choose a reason for hiding this comment

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

Couple of things, but mostly good.

help_to_heat/frontdoor/schemas.py Outdated Show resolved Hide resolved
help_to_heat/frontdoor/views.py Outdated Show resolved Hide resolved
help_to_heat/templates/frontdoor/feedback.html Outdated Show resolved Hide resolved
help_to_heat/templates/frontdoor/feedback.html Outdated Show resolved Hide resolved
@chidin194 chidin194 force-pushed the pc-505-new-feedback-survey branch from 86ffa09 to a7edf84 Compare October 25, 2023 09:05
@chidin194 chidin194 force-pushed the pc-505-new-feedback-survey branch from a7edf84 to 8e947fb Compare October 25, 2023 12:06
@chidin194 chidin194 merged commit 8d6d12c into develop Oct 25, 2023
3 checks passed
@jamiehumphries jamiehumphries deleted the pc-505-new-feedback-survey branch January 19, 2024 13:31
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.

3 participants