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

Simplify usage of ItemIntroduction #8480

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Simplify usage of ItemIntroduction #8480

merged 1 commit into from
Jun 20, 2024

Conversation

chosak
Copy link
Member

@chosak chosak commented Jun 17, 2024

Commit 7cebc54 fixed a bug related to how the ItemIntroduction organism is rendered. This went undiscovered because we didn't have tests for the II.

This commit adds a unit test for the II module and also simplifies its invocation across the various page types.

How to test this PR

Some local URLs to test to verify that this doesn't impact rendering:

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code follows the standards laid out in the CFPB development guidelines

Commit 7cebc54 fixed a bug related to
how the ItemIntroduction organism is rendered. This went undiscovered
because we didn't have tests for the II.

This commit adds a unit test for the II module and also simplifies its
invocation across the various page types.

Some local URLs to test to verify that this doesn't impact rendering:

- http://localhost:8000/about-us/blog/recuperese-y-preparese-financieramente-para-las-tormentas/
- http://localhost:8000/enforcement/actions/pennsylvania-higher-education-assistance-agency-pheaa-dba-american-education-services-or-aes/
- http://localhost:8000/rules-policy/notice-opportunities-comment/commenting-on-notices/
- http://localhost:8000/data-research/research-reports/2012-college-credit-card-agreements/
@@ -38,8 +37,7 @@

<article>
<header>
{% set data = {
'category': page.categories.all(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Note explicit removal of this 'category' parameter here. This wasn't doing anything because the item-introduction.html template wasn't using a value.category parameter, but was getting categories from the page object.

This bug prevents categories from showing up on blog or newsroom posts, and has been broken since #2450 in 2016. See internal https://github.local/CFGOV/platform/issues/3790 that was opened in 2020 about this and closed in 2022.

Having just revisited that code for this PR, I want to flag this for @sonnakim @csebianlander in case there is interest in re-adding categories to those page types.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ambivalent about this. We use categories all the time for both page types, but no one seems to have missed their exclusion from the page template, so I think it's fine to remove it at least for now until such time as we revisit the pages more holistically.

Copy link
Member

Choose a reason for hiding this comment

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

What @csebianlander said - makes sense to me, too

Copy link
Member

@wpears wpears left a comment

Choose a reason for hiding this comment

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

Neat!

@chosak chosak added this pull request to the merge queue Jun 20, 2024
Merged via the queue into main with commit 41244e8 Jun 20, 2024
12 checks passed
@chosak chosak deleted the feature/item-intro-refresh branch June 20, 2024 13: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.

4 participants