SWIS-333: Update account page to be language agnostic#571
SWIS-333: Update account page to be language agnostic#571clarissarichard wants to merge 96 commits intoSWIS-300/language-agnostic-landingfrom
account page to be language agnostic#571Conversation
…age-agnostic-personal
…com/NYPL/nypl-library-card-app into SWIS-317/language-agnostic-personal
…age-agnostic-personal
…com/NYPL/nypl-library-card-app into SWIS-317/language-agnostic-personal
…age-agnostic-personal
…uage-agnostic-address
…age-agnostic-personal
…age-agnostic-personal
…age-agnostic-personal
…uage-agnostic-address
clarissarichard
requested review from
Toxiapo,
milliesavalia,
roseb89 and
russellcullen
Toxiapo
left a comment
Toxiapo
left a comment
There was a problem hiding this comment.
One small question, otherwise looks great!
| <Paragraph m={0}> | ||
| <Trans | ||
| i18nKey="account.termsAndCondition.text" | ||
| components={{ a: <Link variant="external" /> }} |
There was a problem hiding this comment.
Good catch. Link is the DSLink in this context, we just never alias it and we probably should for consistency.
If it's not too much work, could you update this and change the import at the top of the file? Thanks!
import { Checkbox, Link as DSLink } from "@nypl/design-system-react-components";
| .getByText("El nombre de usuario está disponible.") // replace when translation available | ||
| .or(page.getByText("This username is available.")); |
There was a problem hiding this comment.
Do we still not have the translation for this?
There was a problem hiding this comment.
Ooh right, this was around the time of the translation investigation. I know we were looking into consolidating apiErrorMessageTranslations and apiMessageTranslation, so this was temporary. I've updated this with the current translation files and I'll tag you to confirm that's preferred!
| this.page = page; | ||
|
|
||
| const required = appContent?.required || "required"; | ||
| const withRequired = (label: string) => `${label} (${required})`; |
| this.availableUsernameMessage = page | ||
| .getByText(apiTranslations["This username is available."].es) | ||
| .or(page.getByText(apiTranslations["This username is available."].en)); | ||
| this.unavailableUsernameMessage = page | ||
| .getByText( | ||
| apiErrorTranslations[ | ||
| "This username is unavailable. Please try another." | ||
| ].es | ||
| ) | ||
| .or(page.getByText("This username is unavailable. Please try another.")); // en not listed |
There was a problem hiding this comment.
@Toxiapo here are the updated locators referencing the API translation files. We may be able to pass in the lang in the AccountPage object to eliminate the need for the .es and .en, but I feel a bit hesitant changing the structure too much. Thoughts on this?
| this.availableUsernameMessage = page.getByText( | ||
| ERROR_MESSAGES.USERNAME_AVAILABLE | ||
| apiTranslations["This username is available."][lang] || | ||
| "This username is available." | ||
| ); | ||
| this.unavailableUsernameMessage = page.getByText( | ||
| ERROR_MESSAGES.USERNAME_UNAVAILABLE | ||
| apiErrorTranslations["This username is unavailable. Please try another."][ | ||
| lang | ||
| ] || "This username is unavailable. Please try another." | ||
| ); |
There was a problem hiding this comment.
@Toxiapo simpler than I thought it would be!
2 participants
Notes
Noting that all commented tests will be resolved by the end before anything is merged to
main! This will be a bit of a tedious thing to review - apologies in advance 😅I'm going merge each PR incrementally into
SWIS-300/language-agnostic-landingand merge that PR intomain. Please let me know if y'all happen to know a better way to do this!Description
accountpage to refer to translation keysaccountpage tests to loop through languagesTickets:
Motivation and Context
Updates Playwright tests to be language agnostic.
How Has This Been Tested?
Run
npm run playwrightChecklist: