Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Implement page footer with USWDS components #1285
base: main
Are you sure you want to change the base?
feat: Implement page footer with USWDS components #1285
Changes from 53 commits
a014680
4e4fef7
7f300ea
545697e
6f58910
f838337
9f059df
e856157
a82d132
65dacea
1aafd0c
18f0173
60398ed
a5b4502
c47e331
e629882
a7a91ec
d0a4cd4
59dd97e
4bdf994
e2afeed
9f4a2fd
fe545d4
f224268
02e5b61
89a9421
0755ef6
4e3a21a
2d175f1
6bf3149
bb5dcad
886912e
8c3c769
d830c03
5089c7e
98fd9ed
c7846f8
fc7d58e
75f6fd3
d72afea
d43dccc
a9fd5ce
e039620
c13d9c1
13c9cb9
8ff3230
b45d108
3584e15
c149d5b
23850fd
0c78b01
c4f1b24
b5fad30
2267afc
c0d279a
8cfe1ce
6846e20
522dab0
e8a7b14
bda0bde
73eff21
9bbd492
9877502
dc2d615
8b758bf
09babe8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't
layout-root
only used for the legacy way?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, whats the point of having the
hideFooter
prop?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a carry over functionality from the old footer. No real documentation to back up its use. But I would infer that certain instances may not want to show this veda footer for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is for the case that the instance wants to have their own footer. ex. ghg has their own footer https://github.com/US-GHG-Center/veda-config-ghg/blob/bf3e66b7299d474dae01576e850bccaaaca5856c/overrides/components/page-footer/component.tsx#L136
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These mocks are mocking what looks to be used for the Header which includes dropdowns which footer doesn't worry about, we should probably explicity define the config for footer separately. And then test the dynamicness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry.. just catching this now but we need to be decoupling from any parcel/virtual modules/(parcel-resolver-veda) dependency for the new architecture.
Its fine for legacy (because it still depends on parcel) to reference these types from here but for new components for refactor - we should be referencing from this file. Feel free to move those types to a more common directory to be shared between Footer and Header. (Header for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI to help out, there has been an ongoing effort since the very beginning of when this refactor work started and is still going on (see current PR) to remove these veda virtrual modules/parcel dependencies for new architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I updated this import to go from the common
common/types.d.ts
level since these types are leveraged in multiple areas.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think Footer will need DropDowns. Unsure if we have any designs where it includes dropdowns but I've never seen and I dont see the option in storybook here so we should probably remove this
DropdownNavLink
type and then I made a note above about explicitly defining a config just for footer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AliceR and I determined that it makes sense to reuse the main and sub navigation for the footer navigation and strip out the dropdown options functionality within the footer. So we need to maintain the dropdown types to appropriately digest the existing navigation for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes more sense to be explicit hence why we have these config driven nav/menus and why we created it like so but thats fine. I'll add a note for this to be re-looked at in #1323 .