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.

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

Feat: add Discover as an accepted bank card #2691

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Feb 14, 2025

Closes #2680

This PR needs to be merged when we are cleared to do so by Product, after Littlepay's Discover compatibility begins. It updates English and Spanish copy across the user-facing flow and the Help page to include Discover as an accepted bank card. The copy is located at the following locations:

  • Help page
  • Eligibility start
  • Contactless modal
  • Enrollment index
  • CalFresh alert

@lalver1 lalver1 self-assigned this Feb 14, 2025
@github-actions github-actions bot added front-end HTML/CSS/JavaScript and Django templates deployment-dev [auto] Changes that will trigger a deploy if merged to dev i18n Copy: Language files or Django i18n framework labels Feb 14, 2025
Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

@lalver1 lalver1 force-pushed the feat/add-discover-card branch 2 times, most recently from e6bdef9 to c3e4be9 Compare February 18, 2025 16:41
@lalver1 lalver1 marked this pull request as ready for review February 18, 2025 16:58
@lalver1 lalver1 requested a review from a team as a code owner February 18, 2025 16:58
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.

@lalver1 can you please rebase and update these commit messages so they are not all exactly the same? I get that the extended message has a difference, but this is hard to look at if all you're seeing is a list of commit messages:

image

We have the (scope) portion of the commit message for exactly this kind of differentiation, e.g.

feat(help): add Discover as an accepted bank card

@lalver1 lalver1 force-pushed the feat/add-discover-card branch from 0d91edd to c166971 Compare February 18, 2025 17:33
@lalver1
Copy link
Member Author

lalver1 commented Feb 18, 2025

Thanks for the comment @thekaveman, I agree, using the (scope) portion improved the readability 👍

@thekaveman
Copy link
Member

@lalver1 I'm not trying to be harsh here, but I think we should be at a place where commit message formatting isn't really a point of discussion?

The latest messages... also don't really make sense? 3 different versions/lengths, this looks very messy:

image

image

Can we please get some consistency here, assuming that the first line contains the bulk of the information. The extended message only providing additional context where needed.

@angela-tran
Copy link
Member

angela-tran commented Feb 18, 2025

I agree with @thekaveman's comments. Commit message readability is important 👍

Some info that might help here:

Even though Git docs suggest keeping the subject line to no more than 50 characters, GitHub will actually show up to 72 characters of the subject line before collapsing the rest of the message into the "extended description", i.e the body.

For example, the commit message for a1c3d67 has a subject that's 72 characters long.

The Git Graph extension also shows subject lines with more than 50 characters without any problem:
Screenshot from 2025-02-18 12-02-13

I suggest making use of (up to) all 72 characters 🙂

@angela-tran
Copy link
Member

I think it's much more helpful to see a complete sentence as the subject rather than a sentence broken up into a 50-character fragment and having to expand to see the rest of the sentence.

@lalver1 lalver1 force-pushed the feat/add-discover-card branch from 5b1ebbf to 641872a Compare February 18, 2025 18:38
@lalver1
Copy link
Member Author

lalver1 commented Feb 18, 2025

Thanks @thekaveman and @angela-tran, and sorry for spending time on this. The 50 character limit was the reason for the inconsistent subject line length (I focused only on that and forgot about the actual content of the subject line, my bad) but I should've remembered that we can use all 72 characters, so thanks for reminding me about this 🙏

msgid "Your card must include a Visa or Mastercard logo."
msgstr "Su tarjeta debe incluir un logotipo de Visa o Mastercard."
msgid "Your card must include a Visa, Mastercard, or Discover logo."
msgstr "Su tarjeta debe incluir un logotipo de Visa, Mastercard o Discover."
Copy link
Member

Choose a reason for hiding this comment

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

Today I learned (or re-learned, I suppose) that Spanish does not use the Oxford comma. Thank you @lalver1!

image

https://www.spanishdict.com/guide/advanced-spanish-punctuation

Copy link
Member

Choose a reason for hiding this comment

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

🇬🇧 🇬🇧 🇬🇧 🇬🇧 🇬🇧 🇬🇧

Comment on lines +14 to +16
--bs-font-sans-serif:
Roboto, system-ui, -apple-system, "Segoe UI", sans-serif,
"Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji";
Copy link
Member

Choose a reason for hiding this comment

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

Still a mystery why this happens with pre-commit and CSS files

Copy link
Member

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

The copy changes look good to me.

In the future, let's also strive for complete sentences in the commit message body.

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.

Thanks for the updates @lalver1 and comments @angela-tran!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates i18n Copy: Language files or Django i18n framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy update: Discover card
4 participants