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

Create wagtail-editable alternate homepage #8738

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

wpears
Copy link
Member

@wpears wpears commented Feb 6, 2025

UPDATED

This supersedes #8460

This PR updates the home_page model to have a bunch of fields will allow us to capture the homepage content in wagtail. HOWEVER, in this PR, the template remains unchanged, so that we can get the data in the database before switching to the new template.

There is also a new organism CardBlockGroup and a new molecule CardBlock which allow us to use the patterns defined in https://cfpb.github.io/design-system/patterns/cards in wagtail more broadly (we had template implementations but never organism implementations. In order to leave the home_page.html template in place which depends on these, I've temporarily named the new templates cardb.html etc. This will be rectified in a followup PR).

Testing

Deployment

  • Once this is merged and deployed, we'll be able to add data in prod
  • I'll then open an followup PR which switches templates (and renames the silly cardb stuff)
  • When THAT PR is merged, the data already entered will get picked up by the new template and we'll be in business. No downtime, no funny business!

Copy link
Member

@chosak chosak left a comment

Choose a reason for hiding this comment

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

Would you mind also updating the test database (test.sql.gz) so that the "empty" default site contains a valid home page instance?

@wpears
Copy link
Member Author

wpears commented Feb 6, 2025

Would you mind also updating the test database (test.sql.gz) so that the "empty" default site contains a valid home page instance?

@chosak Done. I've also radically altered the deployment plan (see above)

@wpears wpears marked this pull request as ready for review February 6, 2025 21:05
Copy link
Contributor

@csebianlander csebianlander left a comment

Choose a reason for hiding this comment

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

defer to others re: technical and code-polishing details, but the implementation and rollout plan are good with me!

Comment on lines 25 to 46
.content__level--1 {
max-width: 1230px;
margin-left: auto;
margin-right: auto;
}

.content__level--2 {
max-width: 930px;
margin-left: auto;
margin-right: auto;
}

.content__level--3 {
max-width: 870px;
margin-left: auto;
margin-right: auto;
}

.content__main--flush-inner {
padding-top: 0;
margin-top: 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have these four, when they are redundant to what's existing https://github.com/cfpb/consumerfinance.gov/blob/main/cfgov/unprocessed/apps/homepage/css/main.scss ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought these content levels looked better than the other ones, (--1, -1, and -2 vs --1, --2, --3). Everything outside the alternate homepage class will be deleted in a followup PR

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I guess I'm getting confused here on what's temporary and what's not. These classes are mixed up. To be proper BEM, they are saying they would have a parent that is content and then the child element is content__level and then that would have a modifier that is content__level--1, …--2, …--3. However, these don't appear to have a parent content classed container, and the modifier is not adjusting a content__level element. Additionally, I think we should view modifiers on elements as a discouraged pattern. Modifiers are best on the parent block.

If we don't want to add the BEM structures to this, then I would say these are one-off utilities, in which case I would name them u-content-level-1, u-content-level-2, and u-content-level-3. What do you think? Is this something you want to do here or in the subsequent PR? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 83c34e0

.m-info-unit__image{
width:60px;
height:40px;
margin:0 auto .6em 0
Copy link
Member

Choose a reason for hiding this comment

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

Run yarn lint

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in e65130b

@@ -20,3 +20,34 @@
padding-top: 0;
margin-top: 0;
}

.alternate-homepage {
Copy link
Member

Choose a reason for hiding this comment

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

What's the plan with this alternate-homepage class? It doesn't seem like something we want as a final class on the homepage. It appears it is only used to adjust two m-info-unit groups.

Instead of applying this on the container twice in the markup, how about creating a modifier for o-info-unit-group and applying that to the two info unit groups that are being adjusted?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be deleted in the second PR when I remove the hardcoded template

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