-
Notifications
You must be signed in to change notification settings - Fork 115
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
base: main
Are you sure you want to change the base?
Conversation
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.
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) |
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.
defer to others re: technical and code-polishing details, but the implementation and rollout plan are good with me!
.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; | ||
} |
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.
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 ?
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 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
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 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!
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.
Done in 83c34e0
.m-info-unit__image{ | ||
width:60px; | ||
height:40px; | ||
margin:0 auto .6em 0 |
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.
Run yarn lint
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.
fixed in e65130b
@@ -20,3 +20,34 @@ | |||
padding-top: 0; | |||
margin-top: 0; | |||
} | |||
|
|||
.alternate-homepage { |
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.
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?
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.
This will be deleted in the second PR when I remove the hardcoded template
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 moleculeCardBlock
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
./refresh-data test.sql.gz
(this will set your local db to the test db).Deployment