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.

Sign up for GitHub

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

Jump to bottom

feat(appeals): adds dynamic display page rendering #1396

Merged
merged 4 commits into from
Sep 11, 2023
Merged

feat(appeals): adds dynamic display page rendering #1396

merged 4 commits into from
Sep 11, 2023

Conversation

AlexMoulin
Copy link
Contributor

improves and simplifies the dynamic display page rendering, and implements it in Appeals details and HAS LPAQ

Describe your changes

Issue ticket number and link

Type of change 🧩

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Other (please explain in the description section above)

Checklist before requesting a review

  • I have performed a self-review of my own code
  • I have double checked this work does not include any hardcoded secrets or passwords
  • I have made corresponding changes to the documentation
  • I have provided details on how I have tested my code
  • I have referenced the ticket number above
  • I have provided a description of the ticket
  • I have included unit tests to cover any testable code changes

beejjacobs
beejjacobs reviewed Aug 31, 2023
View reviewed changes
Copy link
Contributor

@beejjacobs beejjacobs left a comment

Choose a reason for hiding this comment

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

Have had a quick look through, got some suggestions. As always, just suggestions: you're welcome to disagree or discuss!

In general, this looks great. Well documented types, clear patterns, and I can see this speeding up development.

appeals/web/src/server/lib/display-page-formatter.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/display-page-formatter.js Outdated Show resolved Hide resolved
appeals/web/src/server/views/patterns/display-page.pattern.njk Outdated Show resolved Hide resolved
appeals/web/src/server/lib/nunjucks-template-builders/gds-components-properties-jsdoc.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/nunjucks-template-builders/gds-components-properties-jsdoc.js Show resolved Hide resolved
appeals/web/src/server/lib/mappers/lpaQuestionnaire.mapper.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/mappers/lpaQuestionnaire.mapper.js Show resolved Hide resolved
appeals/web/src/server/lib/mappers/lpaQuestionnaire.mapper.js Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/lpa-questionaire/lpa-questionnaire.mapper.js Outdated Show resolved Hide resolved
shepda
shepda reviewed Aug 31, 2023
View reviewed changes
Copy link
Contributor

@shepda shepda left a comment

Choose a reason for hiding this comment

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

Good work, some pretty small comments

appeals/web/src/server/appeals/appeal-details/appeal-details.controller.js Outdated Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/appeal-details.mapper.js Outdated Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/lpa-questionaire/lpa-questionnaire.controller.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/display-page-formatter.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/display-page-formatter.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/display-page-formatter.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/mappers/appeal.mapper.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/mappers/appeal.mapper.js Outdated Show resolved Hide resolved
james-hill-kc
james-hill-kc requested changes Sep 1, 2023
View reviewed changes
Copy link
Collaborator

@james-hill-kc james-hill-kc left a comment

Choose a reason for hiding this comment

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

Looks really good overall! I've left quite a few comments but mostly minor stuff

appeals/web/src/server/appeals/appeal-details/appeal-details.controller.js Outdated Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/appeal-details.controller.js Outdated Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/appeal-details.controller.js Show resolved Hide resolved
appeals/web/src/server/views/patterns/display-page.pattern.njk Outdated Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/appeal-details.mapper.js Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/lpa-questionaire/lpa-questionnaire.controller.js Show resolved Hide resolved
appeals/web/src/server/appeals/appeal-details/lpa-questionaire/lpa-questionnaire.controller.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/display-page-formatter.js Outdated Show resolved Hide resolved
appeals/web/src/server/lib/display-page-formatter.js Show resolved Hide resolved
appeals/web/src/server/lib/display-page-formatter.js Outdated Show resolved Hide resolved
@AlexMoulin AlexMoulin force-pushed the feat/boat-119-LPAQ-display-page branch 6 times, most recently from 4a5539f to 8b282fc Compare September 11, 2023 14:53
@AlexMoulin AlexMoulin force-pushed the feat/boat-119-LPAQ-display-page branch from 8b282fc to e7a7a56 Compare September 11, 2023 14:54
james-hill-kc
james-hill-kc approved these changes Sep 11, 2023
View reviewed changes
Copy link
Collaborator

@james-hill-kc james-hill-kc left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@AlexMoulin AlexMoulin merged commit 389e2f2 into main Sep 11, 2023
6 checks passed
@AlexMoulin AlexMoulin deleted the feat/boat-119-LPAQ-display-page branch September 11, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabfab-kc fabfab-kc fabfab-kc left review comments

@shepda shepda shepda left review comments

@beejjacobs beejjacobs beejjacobs left review comments

@james-hill-kc james-hill-kc james-hill-kc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlexMoulin @beejjacobs @shepda @james-hill-kc @fabfab-kc