Create wagtail-editable alternate homepage - #8738
Conversation
chosak
left a comment
There was a problem hiding this comment.
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) |
csebianlander
left a comment
There was a problem hiding this comment.
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.
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.
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.
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!
| .m-info-unit__image{ | ||
| width:60px; | ||
| height:40px; | ||
| margin:0 auto .6em 0 |
| margin-top: 0; | ||
| } | ||
|
|
||
| .alternate-homepage { |
There was a problem hiding this comment.
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.
This will be deleted in the second PR when I remove the hardcoded template
UPDATED
This supersedes #8460
This PR updates the
home_pagemodel 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
CardBlockGroupand a new moleculeCardBlockwhich 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