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

MLPAB-307: Donor progress page #292

Merged
merged 30 commits into from
Jan 30, 2023
Merged

MLPAB-307: Donor progress page #292

merged 30 commits into from
Jan 30, 2023

Conversation

acsauk
Copy link
Contributor

@acsauk acsauk commented Jan 25, 2023

Purpose

Adds a vertical progress bar component based on ministryofjustice/moj-frontend#379 and a progress page so a donor can view the progress of their LPA being registered.

Fixes MLPAB-307

Approach

This ticket ballooned a bit as I started to extend/modify some of the existing features of the app:

  • Expanded the fixture endpoints to allow creation of a completed LPA to ease manual testing
  • Removed a duplicated partial for showing attorneys and updated the remaining partial to be a little smarter re: showing actions and handling both Attorney and ReplacementAttorney
  • Updated other actor summary partials to be smarter on when to show actions

Fully aware the /testing-start handler is getting a bit ridiculous now but we've had a need for setting app state since sections have become more rigid on being completed. This could form the basis of a fixture feature in the app where we can put a frontend on completing specific or all sections of the donor flow to make demoing and content/design reviews easier.

Checklist

  • I have performed a self-review of my own code
  • I have added relevant logging with appropriate levels to my code
  • I have updated documentation (Confluence/GitHub wiki/tech debt doc) where relevant
  • I have added tests to prove my work
  • I have added welsh translation tags and updated translation files
  • I have run an accessibility tool on any pages I have made changes to and fixed any issues found
  • The product team have tested these changes

Comment on lines +15 to +20
// IE8 does not support the text justify approach for spacing
@include govuk-if-ie8 {
display: table;
table-layout: fixed;
width: 100%;
}
Copy link
Contributor Author

@acsauk acsauk Jan 25, 2023

Choose a reason for hiding this comment

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

This and other IE8 workarounds are straight from the existing horizontal component CSS and haven't been tested. Thinking maybe to drop them out and this can be revisited if needed when/if we pollinate back to the pattern library?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with us doing a straight copy paste of all this stuff

@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Base: 94.22% // Head: 93.88% // Decreases project coverage by -0.34% ⚠️

Coverage data is based on head (0edbaa2) compared to base (fb9737f).
Patch coverage: 89.00% of modified lines in pull request are covered.

❗ Current head 0edbaa2 differs from pull request most recent head 1d84e55. Consider uploading reports for the commit 1d84e55 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
- Coverage   94.22%   93.88%   -0.34%     
==========================================
  Files          65       66       +1     
  Lines        3894     4022     +128     
==========================================
+ Hits         3669     3776     +107     
- Misses        171      192      +21     
  Partials       54       54              
Flag Coverage Δ
unittests 93.88% <89.00%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/internal/page/data.go 81.53% <29.03%> (-8.11%) ⬇️
app/internal/page/app.go 97.99% <100.00%> (+0.13%) ⬆️
app/internal/page/fixtures.go 100.00% <100.00%> (ø)
app/internal/templatefn/fn.go 92.77% <100.00%> (+0.99%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@acsauk acsauk temporarily deployed to dev_292mlpab307 January 25, 2023 21:25 — with GitHub Actions Inactive
@acsauk acsauk temporarily deployed to dev_292mlpab307 January 26, 2023 09:57 — with GitHub Actions Inactive
@acsauk acsauk changed the title Mlpab 307 MLPAB-307: Donor progress page Jan 26, 2023
@acsauk acsauk marked this pull request as ready for review January 26, 2023 10:16
@acsauk acsauk requested a review from a team as a code owner January 26, 2023 10:16
@acsauk acsauk temporarily deployed to dev_292mlpab307 January 26, 2023 10:34 — with GitHub Actions Inactive
app/internal/page/data.go Outdated Show resolved Hide resolved
app/web/template/dashboard.gohtml Outdated Show resolved Hide resolved
app/web/template/layout/lpa-decisions.gohtml Show resolved Hide resolved
Comment on lines +15 to +20
// IE8 does not support the text justify approach for spacing
@include govuk-if-ie8 {
display: table;
table-layout: fixed;
width: 100%;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with us doing a straight copy paste of all this stuff

case TaskNotStarted:
return "notStarted"
case TaskInProgress:
return "inProgress"
Copy link
Contributor

Choose a reason for hiding this comment

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

should probably use "not started" and "in progress" if we are using these for the visually hidden content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking here was they add context for screen readers given the status is represented as an icon so we'd want to support Welsh for that.

hawx
hawx previously approved these changes Jan 30, 2023
@acsauk acsauk temporarily deployed to dev_292mlpab307 January 30, 2023 11:22 — with GitHub Actions Inactive
@acsauk acsauk merged commit 8ceb858 into main Jan 30, 2023
@acsauk acsauk deleted the MLPAB-307 branch January 30, 2023 11:51
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