-
Notifications
You must be signed in to change notification settings - Fork 9
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
Refactor & Redesign: Eligibility Index, Eligibility Confirm form pages #1023
Conversation
…firm, eligibility index. make form alignment configurable from template
…variables and bootstrap classes
…d variables and bootstrap classes as appropriate
…able-ize/mobile-first styles for Login.gov button
…t Connect Your Card page design
…select template specific label
…(not being used); set page.classes in elig views for index and confirm to style the forms differently
Reviewing changes now 👀 |
|
||
<div class="row justify-content-center"> | ||
<div class="col-lg-10"> | ||
{% block icon_title %} |
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.
Just a comment: since this now inherits from base.html
, there's really no need for these {% block %}
sections -- they are meant to create (or use) placeholders in a parent template.
We could just have the content inside the blocks directly here:
{% block icon_title %}
{% block paragraph_content %}
{% block forms %}
And same comment for eligibility/index.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.
Ended up keeping {% block paragraph_content %} and {% block forms %} and modifying how the nav_buttons work cos it was messing up the width of the 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.
Just curious, why keep those two block
?
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.
Forms has to be block
cos I'm overriding both form and the CTA
I opted to put paragraph content in block
because I wanted to be able to set their own padding for the paragraphs from the inside.
I don't want to set padding on paragraph_content
at the base.html
level, because in the cases where there is a page with a blank paragraph_content
, it will still add padding. And we don't want that.
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 need to override paragraph_content
b/c:
-
I need to add these classes
pt-4 pb-4 pb-lg-8
which set padding-bottom to 64px, among other things, from the inside of the parent div. -
If we add
pt-4 pb-4 pb-lg-8
to the main parent itself, when there are pages withoutparagraph_content
(which most pages don't haveparagraph_content
), the pages will have a giant 64px blank space. Padding applies even if the div inside is empty. We should not be adding padding classes to divs that might be empty/un-used in the base template.
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.
Ok I don't want any of this to be blocking for this PR since it really doesn't matter to the end result...
Forms has to be
block
cos I'm overriding both form and the CTA
There isn't anything to override. {% block %}
is used to place content in specific spot (the block) defined in a parent template. The parent here base.html
does not define a {% block forms %}
, so there is no spot to place/override anything. In confirm.html
, this block turns into... exactly nothing in the rendered HTML.
By using {% block forms %}
here, it creates a block that another template, inheriting from confirm.html
, could fill in; that doesn't really make sense / isn't our use-case for these form pages.
I opted to put paragraph content in
block
because I wanted to be able to set their own padding for the paragraphs from the inside... I don't want to set padding on paragraph_content at the base.html level
So here it looks like a change to put the paragraphs into {% block explanatory-text %}
which makes sense - the base.html
defines this, so this places the paragraphs in that spot in the base template.
This PR is now using templates that inherit from the latest
|
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 really good!
Possible regression?
I'm not sure if this is a minor regression on Mobile, but the "Previous" button should have border-width: 2px
, it went back to 1px
here?
Eligibility Index
Figma - Desktop
This PR running locally - Desktop
Figma - Mobile
This PR running locally - Mobile
Eligibility Confirm
Figma - Desktop
This PR running locally - Desktop
Figma - Mobile
This PR running locally - Mobile
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 started this review earlier today and am just now submitting it.
I feel like I actually want to look at the CSS changes more closely, but don't want to prolong submitting the comments that I do have
Some of my commits on #1013 do the same thing, which is ok - the merging is fine. Not sure if there's a way to more clearly delineate what belongs in which PR. Edit: maybe it's just because this PR and #1013 both involved trying to use the grid, and in the process of doing so, we're going to notice things that need to be tweaked. Fortunately we both were thinking of the same tweaks. |
Today's update:
Happy to go over any more reviews over Zoom! |
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.
Other than @thekaveman's question at #1023 (comment), everything on this PR looks resolved to me
closes #947
What this PR does
This PR shows how the two pages, Eligibility Index and Eligibility Confirm, can be created with a one-off html file that inherit from
page
orbase
and overridesmain_content
, while also allowing the form input area to be properly aligned for its own unique page design.This PR also will refactor the Radio Button and Text Input CSS with variables and Bootstrap classes.
Eligibility Index / Radio Buttons
offset-lg-1 col-lg-9
to the form includes to set the form as 9-col wide and aligned with the headline and explanatory texteligibility/index.html
that inherits frombase
Eligibility Confirm / Text Inputs
offset-lg-3 col-lg-6
to the form includes, to set the form as 6-col wide and centeredeligibility/confirm.html
and add on headline, explanatory-text and formForm template
Form submit buttons
Other
Form logic
classes
key and value within theinit()
function. I useform.classes
to pass in the correct string of CSS classes to get the form classes in. I undid all the Page.classes related changes.page.context_dict()
to setctx["show_help_text"] = True
to show the help text next to the button on the Index page.Screenshots
Eligibility Index
Eligibility Confirm
Now with the Previous Buttons, too ✨ ✨ ✨
Mobile