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 application footer #212

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Update application footer #212

merged 4 commits into from
Dec 10, 2024

Conversation

kyle1morel
Copy link
Collaborator

@kyle1morel kyle1morel commented Dec 5, 2024

Description

Aligning to new style guide

https://apps.nrs.gov.bc.ca/int/jira/browse/PADS-372?

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link

github-actions bot commented Dec 5, 2024

Coverage Report (Frontend)

Totals Coverage
Statements: 24.72% ( 1658 / 6707 )
Methods: 19.89% ( 227 / 1141 )
Lines: 29.43% ( 975 / 3313 )
Branches: 20.24% ( 456 / 2253 )

Copy link

github-actions bot commented Dec 5, 2024

Coverage Report (Application)

Totals Coverage
Statements: 36.25% ( 1047 / 2888 )
Methods: 24.6% ( 123 / 500 )
Lines: 47.55% ( 698 / 1468 )
Branches: 24.57% ( 226 / 920 )

Copy link
Member

@jujaga jujaga left a comment

Choose a reason for hiding this comment

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

Looks pretty nice. A few minor concerns to take a quick look into.

@@ -158,7 +158,7 @@ function updateQueryParams() {
:sort-order="pagination.order"
paginator-template="RowsPerPageDropdown CurrentPageReport PrevPageLink NextPageLink "
current-page-report-template="{first}-{last} of {totalRecords}"
:rows-per-page-options="[10, 20, 50]"
:rows-per-page-options="[10, 20, 50, submissions?.length as number]"
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to behave as a "show all rows" option? The value on this looks like it can change, which could be jarring, or slightly strange if there happen to be exactly 10, 20 or 50 records present.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Request from the business to have an "all" option. Made this dynamic and only appear when needed.

Comment on lines 76 to 79
border-top: 3px solid #fcba19;
border-bottom: 3px solid #fcba19;
background-color: #252423 !important;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is possible, but if we have CSS/SCSS variable constants defined for these colors, it would be good to reference those in our main sheet in lieu of defining them explicitly here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should reference surface 900 or something once the PrimeVue 4 changes come in. I'll leave a TODO on this.

"contact1": "We can help in over 220 languages and through other accessible options.",
"contact2": "Call, email or text us",
"contact3": ", or",
"contact4": "find a service centre.",
Copy link
Member

Choose a reason for hiding this comment

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

The GCPE site did not have a period at the end of this sentence.

@wilwong89 wilwong89 merged commit 5a8f642 into master Dec 10, 2024
18 of 19 checks passed
@wilwong89 wilwong89 deleted the feature/footer branch December 10, 2024 17:46
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