style: Simplify application index page #406

MattRenda · 2022-04-05T23:31:57Z

Resolves #366

Updates the core.pages.index.content_title text in English and Spanish

Removes all paragraph content from the viewmodel in core:index

Updates the two buttons side-by-side

Screenshots

Updates the core.pages.index.content_title text in English and Spanish Removes all paragraph content from the viewmodel in core:index Updates the two buttons side-by-side

benefits/core/templates/core/page.html

machikoyasuda · 2022-04-06T19:40:40Z

Please create and use CSS classes/styles in styles.css, rather than using inline styles. Aside from that, everything looks good to me.

thekaveman

Left a similar comment on @machikoyasuda's review, but restating again:

I think the better approach here is to create a new template (.html file) specifically for the homepage, so you can override the rendering of buttons there - rather than adding a new attribute to the viewmodels for only this one page. It will also let you get the classes in (see Machiko's comments) specifically for these side-by-side homepage buttons.

See e.g. how the help.html template {% extends "core/page.html" %}, then uses the {% block %} definitions from page.html to place content or override content in certain areas.

Then in the view you can specify which template file to use.

Adds index.html which extends page.html Moves inline styling to styles.css

angela-tran

I had a chance to review this today 😄 and left some comments about viewmodel.Page.

angela-tran · 2022-04-08T18:41:51Z

benefits/core/viewmodels.py

@@ -122,6 +123,7 @@ def __init__(self, **kwargs):
        self.noimage = kwargs.get("noimage", False)
        self.icon = kwargs.get("icon")
        self.content_title = kwargs.get("content_title")
+        self.choose_provider = kwargs.get("choose_provider")


thought: This field is really specific to the index page. It seems like the only time we would ever create a viewmodels.Page with choose_provider set to something is the one time in views.index(), whereas with the other fields, they are more generic and apply to multiple pages.

my suggestion here: Let's remove this field and go back to setting the first button's label to get the "Choose your transit provider" text. I see that you defined a new style class to get the right spacing, and that might be why you added this field (to have more flexibility). Instead, I think you could add some code to includes/button.html so that you can provide a class to the <label> if needed. Then in index.html, you would provide button_label_class="provider" (or something like that) similar to the way you provided the button to button.html. (See example code at https://docs.djangoproject.com/en/4.0/ref/templates/builtins/#include)

Let me know if you have any questions or thoughts!

angela-tran · 2022-04-08T18:42:58Z

benefits/core/views.py

@@ -30,6 +30,11 @@ def _index_url():
    return reverse("core:index")


+def _index_choose_provider():
+    """Helper returns the content title for the common index page."""
+    return _("core.pages.index.chooseprovider")


As a part of my earlier comment/suggestion, let's remove this function.

Adds button label for choose provider Adds class to button template

thekaveman

One minor change to the class for the label, to be more descriptive.

In general, please also be mindful of what files/changes are included in your commits. The latest commit included a change to the file tests/cypress/package-lock.json ... we have a long standing issue with this file constantly getting changed/updated and you didn't do anything wrong to cause the change in that file.

But it also wasn't part of what you were working, so the file shouldn't have been included in the commit. Not a big deal this time but please keep it in mind.

thekaveman · 2022-04-13T19:41:59Z

benefits/static/css/styles.css

@@ -200,7 +200,10 @@ footer {
  width: 100%;
  text-align: center !important;
 }
-
+.provider {


This class doesn't say enough about what this is or why we have this style.

I'd like it to be a little more descriptive like .index-agencies-label or similar.

angela-tran

Thanks for addressing the viewmodels.Page field @MattRenda.

While re-reading your changes, I noticed a few other issues and left comments below. I think it would be super helpful to us reviewers if you could break up your changes into more atomic commits. With smaller commits that each have their own specific message, it is easier to understand the intent behind your changes.

angela-tran · 2022-04-13T23:04:24Z

benefits/static/css/styles.css

@@ -177,7 +177,7 @@ footer {
  font-weight: 500;
  margin-bottom: 1rem;
  padding: 1rem;
-  width: 100%;
+  width: 49%;


This is affecting other pages. I think putting width: 100% back and moving width: 49% into your style class should solve it, but please check it.

Comparison of the agency-index page on your branch vs. the dev branch:

angela-tran · 2022-04-13T23:07:30Z

benefits/static/css/styles.css

-    width: 70%;
-    margin-left: 15%;
-    margin-right: 15%;
-  }


Are you sure these are ok to remove? i.e. is it possible there were other pages that should have these styles?

angela-tran · 2022-04-13T23:16:54Z

benefits/core/views.py

@@ -12,7 +12,7 @@

 def PageTemplateResponse(request, page_vm):
    """Helper returns a TemplateResponse using the common page template."""
-    return TemplateResponse(request, "core/page.html", page_vm.context_dict())
+    return TemplateResponse(request, "core/index.html", page_vm.context_dict())


PageTemplateResponse is used in many places, so you are potentially changing a lot more than you should for the effect you are trying to get (to make the index function use the core/index.html template).

The return statement of index is what you should be changing. Feel free to ask me any questions if you feel unsure on this!

I didn't notice this, thanks for catching!

angela-tran · 2022-04-13T23:32:01Z

Also, I don't see this change: Updates the core.pages.index.content_title text in English and Spanish

The screenshot on #366 shows what the title text should be.

angela-tran · 2022-04-14T02:10:22Z

@machikoyasuda offered the perspective that the scope of #366 might be too large for one PR. She worked on breaking it up into smaller tickets.

Render the two buttons side-by-side #431 @machikoyasuda will work on this one!
Remove all paragraph content from the viewmodel in core:index #432
Update the core.pages.index.content_title text in English and Spanish #433
@MattRenda Could you close this PR, and make 2 separate PRs for Remove all paragraph content from the viewmodel in core:index #432 and Update the core.pages.index.content_title text in English and Spanish #433? This will help us review and merge them moreo quickly! Thank you.

Decided we should split this work into smaller parts

Renames provider class as index-agencies-label

style: Simplify application index page

a50c31f

Updates the core.pages.index.content_title text in English and Spanish Removes all paragraph content from the viewmodel in core:index Updates the two buttons side-by-side

MattRenda requested a review from a team as a code owner April 5, 2022 23:31

github-actions bot added the deployment-dev [auto] Changes that will trigger a deploy if merged to dev label Apr 5, 2022

machikoyasuda reviewed Apr 6, 2022

View reviewed changes

benefits/core/templates/core/page.html Outdated Show resolved Hide resolved

thekaveman requested changes Apr 6, 2022

View reviewed changes

refactor: Implements code review feedback

08c5e01

Adds index.html which extends page.html Moves inline styling to styles.css

angela-tran requested changes Apr 8, 2022

View reviewed changes

machikoyasuda added this to the Login.gov SSO milestone Apr 8, 2022

refactor: implements 2nd code review feedback

e002f8d

Adds button label for choose provider Adds class to button template

thekaveman requested changes Apr 13, 2022

View reviewed changes

angela-tran previously requested changes Apr 13, 2022

View reviewed changes

thekaveman removed this from the Login.gov SSO milestone Apr 14, 2022

refactor: implements 3rd code review feedback

f14997b

Renames provider class as index-agencies-label

MattRenda closed this Apr 14, 2022

thekaveman deleted the task366 branch April 14, 2022 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: Simplify application index page #406

style: Simplify application index page #406

MattRenda commented Apr 5, 2022 •

edited by machikoyasuda

Loading

machikoyasuda commented Apr 6, 2022

thekaveman left a comment •

edited

Loading

angela-tran left a comment

angela-tran Apr 8, 2022 •

edited

Loading

angela-tran Apr 8, 2022

thekaveman left a comment

thekaveman Apr 13, 2022

angela-tran left a comment

angela-tran Apr 13, 2022

angela-tran Apr 13, 2022

angela-tran Apr 13, 2022 •

edited

Loading

thekaveman Apr 14, 2022

angela-tran commented Apr 13, 2022

angela-tran commented Apr 14, 2022 •

edited by machikoyasuda

Loading

style: Simplify application index page #406

style: Simplify application index page #406

Conversation

MattRenda commented Apr 5, 2022 • edited by machikoyasuda Loading

Screenshots

machikoyasuda commented Apr 6, 2022

thekaveman left a comment • edited Loading

Choose a reason for hiding this comment

angela-tran left a comment

Choose a reason for hiding this comment

angela-tran Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

angela-tran Apr 8, 2022

Choose a reason for hiding this comment

thekaveman left a comment

Choose a reason for hiding this comment

thekaveman Apr 13, 2022

Choose a reason for hiding this comment

angela-tran left a comment

Choose a reason for hiding this comment

angela-tran Apr 13, 2022

Choose a reason for hiding this comment

angela-tran Apr 13, 2022

Choose a reason for hiding this comment

angela-tran Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

thekaveman Apr 14, 2022

Choose a reason for hiding this comment

angela-tran commented Apr 13, 2022

angela-tran commented Apr 14, 2022 • edited by machikoyasuda Loading

MattRenda commented Apr 5, 2022 •

edited by machikoyasuda

Loading

thekaveman left a comment •

edited

Loading

angela-tran Apr 8, 2022 •

edited

Loading

angela-tran Apr 13, 2022 •

edited

Loading

angela-tran commented Apr 14, 2022 •

edited by machikoyasuda

Loading