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

Chore: Help copy - agency card #1560

Merged
merged 4 commits into from
Jul 26, 2023
Merged

Chore: Help copy - agency card #1560

merged 4 commits into from
Jul 26, 2023

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Jul 21, 2023

Part of #1544

This PR does not change the copy for the "agency card" section of the Help page, but rather extracts that section out to be a template specified by the TransitAgency model.

@angela-tran angela-tran added this to the Veterans milestone Jul 21, 2023
@angela-tran angela-tran self-assigned this Jul 21, 2023
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev i18n Copy: Language files or Django i18n framework front-end HTML/CSS/JavaScript and Django templates labels Jul 21, 2023
@angela-tran angela-tran force-pushed the chore/help-copy-agency-card branch 2 times, most recently from 548b9c9 to 0b47a39 Compare July 25, 2023 21:23
@angela-tran
Copy link
Member Author

I'm not super sure about this PR... I was just following our pattern of using placeholders for variables that don't exist on models, but realistically, it'll be a while before we have another agency that this section applies to. So are we going to have another PR later that essentially reverts this one?

@angela-tran angela-tran marked this pull request as ready for review July 25, 2023 21:27
@angela-tran angela-tran requested a review from a team as a code owner July 25, 2023 21:27
@thekaveman
Copy link
Member

I'm not super sure about this PR... I was just following our pattern of using placeholders for variables that don't exist on models, but realistically, it'll be a while before we have another agency that this section applies to. So are we going to have another PR later that essentially reverts this one?

Yeah this doesn't feel all that useful to introduce the placeholders when all this text is known for MST/Courtesy Cards.

@thekaveman
Copy link
Member

thekaveman commented Jul 25, 2023

Maybe a change is to refactor the help page to optionally show that section, with a TransitAgency.help_template (or agency_card_info_template or something) field that points to an include? (To get this ready for e.g. SacRT)

@angela-tran angela-tran force-pushed the chore/help-copy-agency-card branch 2 times, most recently from 9245a1b to 4efdcea Compare July 26, 2023 20:44
@angela-tran angela-tran force-pushed the chore/help-copy-agency-card branch from 4efdcea to bc92aef Compare July 26, 2023 20:50
@angela-tran
Copy link
Member Author

angela-tran commented Jul 26, 2023

Maybe a change is to refactor the help page to optionally show that section, with a TransitAgency.help_template (or agency_card_info_template or something) field that points to an include? (To get this ready for e.g. SacRT)

@thekaveman I took your suggestion to do this refactor instead of having placeholder-filled copy. I force-pushed a new set of commits to this branch, and this is ready for re-review

@thekaveman
Copy link
Member

This is looking really great otherwise! 👍

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.

Nice!

@angela-tran angela-tran merged commit 7c246ff into dev Jul 26, 2023
@angela-tran angela-tran deleted the chore/help-copy-agency-card branch July 26, 2023 21:59
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.

2 participants