-
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
Feat: add Discover as an accepted bank card #2691
base: main
Are you sure you want to change the base?
Conversation
e6bdef9
to
c3e4be9
Compare
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.
@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:
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
to the Help page
0d91edd
to
c166971
Compare
Thanks for the comment @thekaveman, I agree, using the |
@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: 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. |
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: I suggest making use of (up to) all 72 characters 🙂 |
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. |
to the Eligibility start page
to the Contactless modal
to the Enrollment index page
to the CalFresh alert
5b1ebbf
to
641872a
Compare
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." |
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.
Today I learned (or re-learned, I suppose) that Spanish does not use the Oxford comma. Thank you @lalver1!
https://www.spanishdict.com/guide/advanced-spanish-punctuation
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.
🇬🇧 🇬🇧 🇬🇧 🇬🇧 🇬🇧 🇬🇧
--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"; |
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.
Still a mystery why this happens with pre-commit and CSS files
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.
The copy changes look good to me.
In the future, let's also strive for complete sentences in the commit message body.
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.
Thanks for the updates @lalver1 and comments @angela-tran!
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: