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

USWDS-Site - Changelog: Update documentation and add entry for in-page nav custom content [USWDS#5387] #2190

Merged
merged 15 commits into from
Aug 18, 2023

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Jul 17, 2023

Summary

Preview link

Problem statement

PR uswds/uswds#5387 added a new data attribute that should be added to the documentation.

Testing and review

In both the properties table and changelog, confirm:

  • Grammar and spelling
  • The updated information is accurate

- Remove element column from properties table
- Instead added a note about adding props to usa-in-page-nav
@amyleadem amyleadem changed the base branch from main to release-3.5.1 July 21, 2023 19:18
@amyleadem amyleadem marked this pull request as ready for review July 28, 2023 17:08
Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

This looks good, added an optional comment to include additional guidance on the new data attribute and functionality. Lmk what you think!

_components/in-page-navigation/guidance/implementation.md Outdated Show resolved Hide resolved
@mejiaj mejiaj changed the title USWDS-Site: Update documentation and add changelog for #5387 USWDS-Site - Changelog: Update documentation and add entry for in-page nav custom content [USWDS#5387] Aug 8, 2023
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Minor suggestion added. Agree with @mahoneycm's comment about adding to guidance #2190 (comment).

@amyleadem
Copy link
Contributor Author

@mahoneycm @mejiaj
I pulled some copy out of the changelog and into the main guidance in 22b77f8 and dbe4035. I also rearranged the content a bit because it felt confusing that we had an intro for init properties after we started talking about init properties. Let me know what you think!

Copy link
Contributor

@mahoneycm mahoneycm left a comment

Choose a reason for hiding this comment

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

Lgtm! Thanks for those updates!

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Very thorough and detailed docs! Added some suggestions to simplify language and improve consistency.

_components/in-page-navigation/guidance/implementation.md Outdated Show resolved Hide resolved
_components/in-page-navigation/guidance/implementation.md Outdated Show resolved Hide resolved
Comment on lines 23 to 27
{% include settings-table-flex.html
content=page.implementation.initProps
cols="flex-2, flex-1, flex-2, flex-1"
cols="flex-2, flex-2, flex-1"
section="initialization properties"
%}
Copy link
Contributor

Choose a reason for hiding this comment

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

This table could use a header to keep it consistent with table below.

Example
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Header added in 02a75dc. I also adjusted the column width in this commit because adding a header caused the table to span the width of the content area, rather than being constrained to the width of the content list, and I wanted to give a little extra room to the description column.

amyleadem and others added 2 commits August 11, 2023 11:05
Co-authored-by: James Mejia <james.mejia@gsa.gov>
@mejiaj mejiaj deleted the branch release-3.6.0 August 11, 2023 20:52
@mejiaj mejiaj closed this Aug 11, 2023
@amyleadem amyleadem reopened this Aug 11, 2023
@amyleadem amyleadem changed the base branch from release-3.5.1 to release-3.6.0 August 11, 2023 21:10
@amyleadem
Copy link
Contributor Author

Thanks @mejiaj. I've made the requested updates. Let me know if you need anything else.

@amyleadem amyleadem requested a review from mejiaj August 11, 2023 21:26
Copy link
Contributor

@mejiaj mejiaj 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. Just added a few notes for future issues.

@mejiaj mejiaj requested a review from thisisdano August 15, 2023 17:02
@thisisdano thisisdano merged commit 364bc64 into release-3.6.0 Aug 18, 2023
5 of 8 checks passed
@thisisdano thisisdano deleted the al-changelog-5387 branch August 18, 2023 17:42
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.

None yet

4 participants