USWDS-Site: Combine "What's new" and "News and updates pages #2958
Conversation
amyleadem
changed the title
| - /whats-new/updates/ | ||
| - /about/updates/ |
There was a problem hiding this comment.
Note
These were redirects from the deleted updates.md file
_includes/nav/current.html
Outdated
| {% elsif page.collection and include.page.subnav.type == page.collection %} | ||
| {% assign _current = true %} | ||
| <!-- | ||
| Use the page's `type` data key to connect the sidenav to a collection. | ||
| This sets "current" for the pages inside the _posts directory. | ||
| Collections are defined in config.yml, and "posts" is a default collection type in Jekyll. | ||
| --> | ||
| {% elsif page.collection and include.page.type == page.collection %} | ||
| {% assign _current = true %} |
There was a problem hiding this comment.
Note
This change allows us to set the "current" page in the sidenav of all our posts to be the "All news and updates" page.
Notes:
- To connect the pages, we needed to also add
type: poststo the front matter of the What's new pages. - The
typedata key is used in the front matter across the site, buttype: postsis not yet used in the repo.
Why I didn't use the existing subnav.type check
Currently, the site connects the posts to the "News and updates" page in the side navigation by using the conditional in lines 7-8 here, along with adding subnav.type = "posts" on the updates.md file.
However, I ran into some errors with this approach when I tried to add both a type and a link item to "All news and updates" inside the subnav in pages/whats-new/overview.md. It caused conflict that caused the side navigation to break (Please confirm that you are not able to get it to work either).
Needs discussion
Proposed additional change
I recommend we remove the subnav.type check in lines 7-8 and replace it with the check added in lines 14-15.
Currently, there is only one other page that uses subnav.type: security.md. Replacing subnav.type with type here should still connect it to the expected collection in config.yml.
I don't see anything in the repo relating to page.type that would cause any regressions, but of course we would want to test that. If we move forward with this approach, we would also need to make an update to the front matter in security.md.
Curious to hear thoughts on this approach and proposed additional change.
| {% elsif page.collection and include.page.subnav.type == page.collection %} | |
| {% assign _current = true %} | |
| <!-- | |
| Use the page's `type` data key to connect the sidenav to a collection. | |
| This sets "current" for the pages inside the _posts directory. | |
| Collections are defined in config.yml, and "posts" is a default collection type in Jekyll. | |
| --> | |
| {% elsif page.collection and include.page.type == page.collection %} | |
| {% assign _current = true %} | |
| <!-- | |
| Use the page's `type` data key to connect the sidenav to a collection. | |
| This sets "current" for the pages inside the _posts directory. | |
| Collections are defined in config.yml, and "posts" is a default collection type in Jekyll. | |
| --> | |
| {% elsif page.collection and include.page.type == page.collection %} | |
| {% assign _current = true %} |
There was a problem hiding this comment.
Note
This change allows us to set the "current" page in the sidenav of all our posts to be the "All news and updates" page.
Notes:
- To connect the pages, we needed to also add
type: poststo the front matter of the What's new pages.- The
typedata key is used in the front matter across the site, buttype: postsis not yet used in the repo.
Would setting type be necessary since we have that setup in defaults?
Lines 92 to 96 in 849d779
There was a problem hiding this comment.
This is somewhere I was also getting caught up while testing. I think it would be a good idea to normalize this across pages. It would also reduce complexity of this current page logic which would be a win!
Praise: Thank you for breaking this down in a clear and easy to follow way!
There was a problem hiding this comment.
Update: I opened issue #2974 to explore this conversation so that we can prevent scope creep here.
There was a problem hiding this comment.
Thanks for consolidating this and the notes. The changes look good and the reasoning to consolidate make a lot of sense.
Added a question and a comment. Ideally, we should move away from the pattern of embedding chunks of HTML in markdown. The larger the chunk the harder it is to update/maintain and could introduce code duplication.
_includes/nav/current.html
Outdated
| {% elsif page.collection and include.page.subnav.type == page.collection %} | ||
| {% assign _current = true %} | ||
| <!-- | ||
| Use the page's `type` data key to connect the sidenav to a collection. | ||
| This sets "current" for the pages inside the _posts directory. | ||
| Collections are defined in config.yml, and "posts" is a default collection type in Jekyll. | ||
| --> | ||
| {% elsif page.collection and include.page.type == page.collection %} | ||
| {% assign _current = true %} |
There was a problem hiding this comment.
Note
This change allows us to set the "current" page in the sidenav of all our posts to be the "All news and updates" page.
Notes:
- To connect the pages, we needed to also add
type: poststo the front matter of the What's new pages.- The
typedata key is used in the front matter across the site, buttype: postsis not yet used in the repo.
Would setting type be necessary since we have that setup in defaults?
Lines 92 to 96 in 849d779
There was a problem hiding this comment.
Barring other potential changes, this is looking good! I left one non-blocking recommendation for code consistency.
- Confirm links work
- Confirm pages display as expected at all screen widths
- Confirm changelogs are present and accurate
- Confirm sidenav highlights the correct current page on the what's new pages and posts
- Confirm redirects work as expected
- Confirm the previous link to "News and updates" (/about/updates/) now points to the "What's new" page
- Confirm the permalinks and url directory structure makes sense for the new pages and posts
- Confirm no regressions in sidenav on different page types.
- Confirm the code meets USWDS standards (pending other requested changes)
_includes/nav/current.html
Outdated
| {% elsif page.collection and include.page.subnav.type == page.collection %} | ||
| {% assign _current = true %} | ||
| <!-- | ||
| Use the page's `type` data key to connect the sidenav to a collection. | ||
| This sets "current" for the pages inside the _posts directory. | ||
| Collections are defined in config.yml, and "posts" is a default collection type in Jekyll. | ||
| --> | ||
| {% elsif page.collection and include.page.type == page.collection %} | ||
| {% assign _current = true %} |
There was a problem hiding this comment.
This is somewhere I was also getting caught up while testing. I think it would be a good idea to normalize this across pages. It would also reduce complexity of this current page logic which would be a win!
Praise: Thank you for breaking this down in a clear and easy to follow way!
…mbine-whats-new-pages
There was a problem hiding this comment.
@amyleadem nice job, the new component looks great and looking forward to seeing more re-usable components.
Added minor comments (mostly polish). Tried to make them easy to incorporate via suggestions. If you don't want to accept, then feel free to resolve with a comment saying Decided against or something to that effect.
I can approve once all comments are resolved.
Co-authored-by: James Mejia <[email protected]>
|
Great idea to include comments in the includes, @mejiaj. I committed both of your suggestions. This is ready for re-review. |
There was a problem hiding this comment.
Loving the flow of the page and the new card includes look fantastic! Great work here @amyleadem 🥳
updated changelog date, assuming we'll wrap tomorrow
annepetersen
dismissed stale reviews from mahoneycm and finekatie
via
e02741b
…mbine-whats-new-pages
|
Update: I fixed the build errors on this PR. All checks are now passing. |
Summary
Combined "What's new" and "News and updates" pages.
This is a follow-up to the prototype work in #2855.
Note
This PR does not include the prototype's updates to the excerpt link structure in
post.html. It was not clear how to accommodate external links in the header links, so I bumped it for now to prevent this from being blocked. We are going to address this in a fast follow-on in #2953. We will also work on further enhancing this layout in #2967.Important
We need to update the changelog dates before merge
Important
This PR is failing because of HTML proofer errors that will be fixed in these PRs:
Related issue
Closes #2915
Follow up issues:
Preview links
Problem statement
Solution
Testing and review
All:
Content:
Devs: