Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enrollment Success: Copy, illo, redesign #1508

Merged
merged 19 commits into from
Jul 12, 2023
Merged

Enrollment Success: Copy, illo, redesign #1508

merged 19 commits into from
Jul 12, 2023

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Jul 10, 2023 •
edited
Loading

closes #1432

What this PR does

  • Delete all expiry item copy + code
  • Delete help link + code
  • Update copy + Spanish
  • Remove media item, use two column layout
  • Add image
  • Mobile
  • Copy: Makes the Enrollment Success body copy generic for all flows. If and when in the future, there is different body copy for specific flows (like, for example, SacRT-Senior, to change transit to light rail), the code can be updated to customize just that particular flow's copy/template.

Note: This PR does not entirely re-do the success.html template to the new style. This PR merely removes as much viewmodel as possible (media item, page for title, headline), but still uses a viewmodel for the Log Out button. Since the body copy is the same across all 4 flows for now (with all the [brackets]), the success.html template points to the same 1 copy for now. Once we have differing copy for various flows (like, maybe, SacRT-Senior), this template can be refactored to show different body copy per flow.

How to test

  • Test CC flow
  • Test Senior flow: I manually commented out the ifs in the success.html file to manually make the Login.gov button line show up. Not sure how else to do it locally.
  • Test VA.gov flow
  • Elig Start - No regressions

Screenshots

No sign out

image image

Sign out

image image

@github-actions github-actions bot added i18n Copy: Language files or Django i18n framework deployment-dev [auto] Changes that will trigger a deploy if merged to dev back-end Django views, sessions, middleware, models, migrations etc. migrations [auto] Review for potential model changes/needed data migrations updates labels Jul 10, 2023
@machikoyasuda machikoyasuda changed the title feat(enrollment success): remove Expiry Item, copy, model fields Enrollment Success: Copy, illo, redesign Jul 10, 2023
@thekaveman thekaveman mentioned this pull request Jul 10, 2023
Merged
@machikoyasuda machikoyasuda force-pushed the feat/1432-veterans--enrollment-success branch from 30acdb2 to 0003905 Compare July 10, 2023 22:22
@machikoyasuda machikoyasuda added this to the Veterans milestone Jul 10, 2023
@machikoyasuda machikoyasuda marked this pull request as ready for review July 10, 2023 23:29
@machikoyasuda machikoyasuda requested a review from a team as a code owner July 10, 2023 23:29
thekaveman
thekaveman previously requested changes Jul 10, 2023
View reviewed changes
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/cal-itp/benefits/pull/1508/files#r1259013328

thekaveman
thekaveman reviewed Jul 10, 2023
View reviewed changes
benefits/enrollment/templates/enrollment/success.html Outdated Show resolved Hide resolved
benefits/core/models.py Outdated Show resolved Hide resolved
@thekaveman thekaveman mentioned this pull request Jul 10, 2023
Closed
@machikoyasuda machikoyasuda force-pushed the feat/1432-veterans--enrollment-success branch from dc02dad to 1a69f50 Compare July 11, 2023 00:18
@machikoyasuda
Copy link
Member Author

Finally ready for re-review @thekaveman -- all the enrollment_success_confirm_item_details variables, etc. are removed and the copy is consolidated to use a generic "enrollment.pages.success.confirm_item.details"

Rebased and re-ran bin/makemessages.sh to ensure django.po file is formatted correctly.

machikoyasuda
machikoyasuda commented Jul 11, 2023
View reviewed changes
benefits/static/css/styles.css
Comment on lines +377 to +380
#login .fallback-text.color-logo {
background-image: url("/static/img/login-gov-logo.svg");
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angela-tran For when you start coding up these 3 - modal trigger link, modal title and modal title - you can use #login .fallback-text.color-logo. Currently #login .fallback-text has a set width and height, but you can edit the CSS to become more generic (remove the width/height from the main class) and add the width/height to a specific class, to be used in those 3 spots.
image
image
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @machikoyasuda!

@thekaveman
Copy link
Member

I think this conflict is pretty minor, just the timestamp in the header section.

@thekaveman thekaveman mentioned this pull request Jul 11, 2023
Closed
3 tasks
@machikoyasuda machikoyasuda force-pushed the feat/1432-veterans--enrollment-success branch from 03e76e6 to 2fcc2a2 Compare July 12, 2023 02:16
@machikoyasuda
Copy link
Member Author

@thekaveman @angela-tran Rebased with the latest dev - Ready for re-review

@machikoyasuda machikoyasuda enabled auto-merge July 12, 2023 02:25
@angela-tran
Copy link
Member

@machikoyasuda Some small merge conflicts to resolve

@machikoyasuda
refactor(enrollment-success): use translate directly, not Page model …
5c126cd 
…for headline, title
@machikoyasuda
fix(enrollment-success): make headline col-lg-8 for better line break
58bf1c1
@machikoyasuda
feat(enrollment-success): add image
f3b6cc4
@machikoyasuda
fix(illo): upgrade illo quality to 2x
a9436ab
@machikoyasuda
chore: run makemessages
7fad459
@machikoyasuda
fix(enrollment-success): 24px between paragraphs
344807a
@machikoyasuda
fix(enrollment-success): desktop - headline to img is 48px; img to p-…
76f076d 
…top is 40px
@machikoyasuda
feat(enrollment-success): mobile - alignment, img sizing
1fc6de1
@machikoyasuda
fix: 180 not 189
26ee58a
@machikoyasuda
feat(enrollment-success): style new Login.gov sign out button
11d5504
@machikoyasuda
feat(enrollment-success): remove confirm_item_details from model, rem…
daceaf7 
…ove flow-specific details msgid
@machikoyasuda
fix: delete phantom code
68d22b4
@machikoyasuda
chore: run flake8
b86a203
@machikoyasuda
fix(copy): remove duplicate msgid definition
2c90080
@machikoyasuda
chore: makemessages
aaad6d0
@machikoyasuda machikoyasuda force-pushed the feat/1432-veterans--enrollment-success branch from 2fcc2a2 to aaad6d0 Compare July 12, 2023 02:55
@machikoyasuda
Copy link
Member Author

@angela-tran Re-re-re-based!

@machikoyasuda machikoyasuda disabled auto-merge July 12, 2023 03:15
@machikoyasuda machikoyasuda merged commit 3a1baca into dev Jul 12, 2023
@machikoyasuda machikoyasuda deleted the feat/1432-veterans--enrollment-success branch July 12, 2023 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angela-tran angela-tran angela-tran approved these changes

@thekaveman thekaveman Awaiting requested review from thekaveman

Assignees

@machikoyasuda machikoyasuda

Labels
back-end Django views, sessions, middleware, models, migrations etc. deployment-dev [auto] Changes that will trigger a deploy if merged to dev i18n Copy: Language files or Django i18n framework migrations [auto] Review for potential model changes/needed data migrations updates
Projects
None yet
Milestone
Veteran rider enrollment pathway
Development

Successfully merging this pull request may close these issues.

Enrollment Success - Add illustration, update copy
3 participants
@machikoyasuda @thekaveman @angela-tran