-
Notifications
You must be signed in to change notification settings - Fork 153
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
Generating a generic listing page and blog listing specific page #3427
Conversation
@kristinashu this should be ready for design review |
network-api/networkapi/wagtailpages/templates/wagtailpages/listing_page.html
Outdated
Show resolved
Hide resolved
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 can tell you just by looking that the margins in the top section are wonky, you probably need a few .my-#
classes in there or something like that. @kristinashu What's the best way to iron this out?
Thanks Pomax! A few questions and thoughts: TagsFor the /blog page, can we show the first tag listed in the posts promote tab for each post? Filter viewHave you added the filter by tag view yet? I added the tag 'test' to one of the post then tried the url https://foundation-mofostaging-pr-3427.herokuapp.com/en/blog-list/?tag=[test] but it didn't change anything. SpacingJust a bit of finessing is needed for the gaps. I'm reluctant to give exact px spacing but here is a comp with some guidelines. I know this will very slightly because of the preexisting spacing associated with the text so just use this comp as a guideline. Note, we stick to multiples of 8px. |
(Let me know if I should review this sooner. I usually wait for PR to pass design review first.) |
@kristinashu fixed the line widths, it should now be 8 at medium and above, and 12 at smaller than that. |
The grey bar looks like the minisite nav without any content (because there is none) so I'll see if I can remove that nav from this template entirely. |
it was - removed the minisite-specific bits because this is not a minisite and what were they doing in the template in the first place here =P |
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.
w00t!
@kristinashu @mmmavis the Percy failure looks like a genuine concern: the "participation guidelines" gets spread out over two columns... |
Why is this PR changing the footer? |
Your guess is as good as mine: it's only showing changes in the footer for two (out of eleven?) pages, both entirely unrelated to this, and if I check those pages on the review app things look perfectly fine, so.... it could just be Percy being wrong? =/ |
Hmm weird, should we push to staging and then check those pages there? |
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.
Two questions:
- In order for a page to show up on an index page, that page has to be child of the index page. This means child page urls will become
/index-page/child-page
. Will we running into issues with URL if we are planning to create index page of existing pages on production (e.g., their URL will change and we'll have to deal with redirects???)? - Will there be cases where we want a page to belong to more than one index page?
network-api/networkapi/wagtailpages/templates/wagtailpages/fragments/generic-card.html
Outdated
Show resolved
Hide resolved
We shouldn't; We've had mini-site namespace pages in place for a while, so this will be replacing some of those, making previously dead URLs into real ones.
I think that's beyond the scope of intent for this page. |
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 looks way better and feels like the right approach. One tiny comment from me and another from Mavis, and I think we're all set.
network-api/networkapi/wagtailpages/templates/wagtailpages/fragments/generic-card.html
Outdated
Show resolved
Hide resolved
…gments/generic-card.html
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.
🎉
Closes #3370
Related PRs/issues #3371
This does not do any fancy "load initial subset, JS-enabled MORE button to load more results", given that the first use of this model is for blog posts, of which we only have a handful.
#3443 has been filed for the JS functionality.
Test
https://foundation-mofostaging-pr-3427.herokuapp.com/en/blog/