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

Licence Summary Page - Summary Tab #631

Merged
merged 21 commits into from
Jan 17, 2024
Merged

Conversation

robertparkinson
Copy link
Collaborator

@robertparkinson robertparkinson commented Jan 3, 2024

https://eaflood.atlassian.net/browse/WATER-4311

Migrating the licence summary page from the water-abstraction-ui to the water-abstraction-system.

robertparkinson and others added 2 commits January 3, 2024 15:20
Migrating the licence summary page from the water-abstraction-ui to the water-abstraction-system.

1
@Cruikshanks Cruikshanks added the enhancement New feature or request label Jan 7, 2024
@robertparkinson robertparkinson marked this pull request as ready for review January 16, 2024 13:23
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

Woohoo! We are finally going to take on the mess that is the legacy view licence page! Fantastic start.

There are some changes I'd like to make (and more reasons to get off my 🍑 and update our conventions 🤦😩!) There are also some things I haven't thought of that are worth having a discussion about.

But there is an overall thought I think I have about the change. Currently, the service and presenter built treat the licence page and summary tab as one-and-the-same.

But when we come to the other tabs they will share some things currently being used, but not all.

So, is it worth at this stage splitting the work needed to fetch and present data for the licence page from that needed for the summary tab?

app/controllers/licences.controller.js Outdated Show resolved Hide resolved
app/views/view-licences/licence.njk Outdated Show resolved Hide resolved
app/presenters/view-licences/view-licence.presenter.js Outdated Show resolved Hide resolved
app/presenters/view-licences/view-licence.presenter.js Outdated Show resolved Hide resolved
app/routes/licence.routes.js Outdated Show resolved Hide resolved
app/services/licences/fetch-licence.service.js Outdated Show resolved Hide resolved
app/services/licences/fetch-licence.service.js Outdated Show resolved Hide resolved
app/services/licences/fetch-licence.service.js Outdated Show resolved Hide resolved
app/views/view-licences/licence.njk Outdated Show resolved Hide resolved
app/views/view-licences/licence.njk Outdated Show resolved Hide resolved
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

Just a few minor tweaks and queries from the second pass.

But the key one is we need to have a discussion about using Nunjucks vs HTML from the design system before this gets merged.

app/presenters/licences/view.presenter.js Outdated Show resolved Hide resolved
app/presenters/licences/view.presenter.js Outdated Show resolved Hide resolved
app/presenters/licences/view.presenter.js Outdated Show resolved Hide resolved
app/presenters/licences/view.presenter.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/controllers/licences.controller.test.js Outdated Show resolved Hide resolved
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

@robertparkinson robertparkinson merged commit 657dc29 into main Jan 17, 2024
6 checks passed
@robertparkinson robertparkinson deleted the licence-summary-page branch January 17, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants