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: in-page navigation #2551
feat: in-page navigation #2551
Changes from 23 commits
e496788
c3f63d2
63cb987
2083469
69d77b0
0e09051
05bc5f3
a631909
ade5417
ddabf58
d5449bd
78aaf40
8be2d6e
069913c
4b939e9
220f0d0
73401b8
787f100
0aa1e7a
ce17d25
8d45f29
d0659de
91799f8
908ca78
520b258
ed166dd
703e2fe
333c8f3
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.
nit: this doesn't appear to be used
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.
ah yep, this was from my previous approach. Removed!
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 the classnames here are specific to the USWDS example. I could definitely see a case where a consumer wouldn't want
usa-prose
applied to their app's page'smain
content for example. Similarly, they might want to customize things aboutmain
like theid
, etc. I've got another comment with further thoughts about making this more flexible for consumers.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.
that comment: #2551 (comment)
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.
addressed in latest commit
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.
question: I have some mixed thoughts on whether we should include the
main
tag here, or expect consumers to pass it in.The USWDS guidance is very clear that this component should interact with well structured markup, so enforcing the use of
main
either way is important:On one hand, passing it in makes using the component more complex. On the other hand, it is more flexible, especially since it's pretty common for React web apps to build
main
into their page components.At the very least,
main
needs to be customizable, so you'd have to at least exposemainProps
in the same way I made the suggestion aboutnavProps
. Then we can just spread{...mainProps}
on it, and (if needed) apply theoffsetStyle
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.
addressed in latest commit