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

Update list collector content page to use list component #1490

Merged
merged 43 commits into from
Sep 23, 2024

Conversation

VirajP1002
Copy link
Contributor

@VirajP1002 VirajP1002 commented Aug 30, 2024

What is the context of this PR?

This PR updates the list collector content file to use the new summary list variant instead of using the summary component. This will solve a DAC issue where list collectors should be marked up as a list in the HTML to aid screen readers.

How to review

  • Use the test_list_collector_content_page schema
  • Navigate until you get to the list collector content page to see the list collector with only one item on each line
  • Test list collector with screen reader
  • Answer questions for one of the items and see the green tick be added
  • Test list collector with green tick with screen reader

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

@rmccar rmccar changed the title Update list collector content page to use new Summary Component Update list collector content page to use list component Sep 3, 2024
@rmccar rmccar marked this pull request as ready for review September 10, 2024 15:51
@rmccar rmccar marked this pull request as draft September 10, 2024 15:51
@rmccar rmccar marked this pull request as ready for review September 12, 2024 09:37
petechd
petechd previously approved these changes Sep 17, 2024
@rmccar rmccar force-pushed the update-list-collector-content-page branch from 4ca4f91 to 7479c54 Compare September 17, 2024 09:27
@berroar
Copy link
Contributor

berroar commented Sep 17, 2024

Only minor issue I notice is that the spacing is now slightly different on the content page, when compared with what it was previously (using test env for example).

Item 2 is slightly indented under the first row.

Test on the left vs this branch:
Screenshot 2024-09-17 at 10 40 10

@rmccar
Copy link
Contributor

rmccar commented Sep 17, 2024

Only minor issue I notice is that the spacing is now slightly different on the content page, when compared with what it was previously (using test env for example).

Item 2 is slightly indented under the first row.

Test on the left vs this branch: Screenshot 2024-09-17 at 10 40 10

Yeah the spacing on the list component is slightly different to the summary one, ill try run this by a designer. I think this is something that can be fixed (if needed) outside of this piece of work though. If any change is needed then this will be a DS change

@berroar
Copy link
Contributor

berroar commented Sep 17, 2024

Only minor issue I notice is that the spacing is now slightly different on the content page, when compared with what it was previously (using test env for example).
Item 2 is slightly indented under the first row.
Test on the left vs this branch: Screenshot 2024-09-17 at 10 40 10

Yeah the spacing on the list component is slightly different to the summary one, ill try run this by a designer. I think this is something that can be fixed (if needed) outside of this piece of work though. If any change is needed then this will be a DS change

Just the acceptance criteria explicitly calls out that it needs to be visually identical so worth checking

@rmccar
Copy link
Contributor

rmccar commented Sep 17, 2024

Only minor issue I notice is that the spacing is now slightly different on the content page, when compared with what it was previously (using test env for example).
Item 2 is slightly indented under the first row.
Test on the left vs this branch: Screenshot 2024-09-17 at 10 40 10

Yeah the spacing on the list component is slightly different to the summary one, ill try run this by a designer. I think this is something that can be fixed (if needed) outside of this piece of work though. If any change is needed then this will be a DS change

Just the acceptance criteria explicitly calls out that it needs to be visually identical so worth checking

Raised a PR on the DS to fix this

app/jinja_filters.py Outdated Show resolved Hide resolved
@rmccar rmccar dismissed petechd’s stale review September 18, 2024 13:17

Needs another review

@rmccar rmccar merged commit bf5821f into main Sep 23, 2024
16 checks passed
@rmccar rmccar deleted the update-list-collector-content-page branch September 23, 2024 08: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.

4 participants