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

In Facility task cards, only quote the facility name and not the ID fragment #8093

Merged

Conversation

jonboiser
Copy link
Contributor

Summary

Fixes #7123

References

Reviewer guidance

The tests should confirm the new behavior, but this can be manually confirmed by importing/syncing/deleting a facility in the app.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@jonboiser jonboiser added changelog Important user-facing changes APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) labels May 19, 2021
@jonboiser jonboiser added this to the 0.15.0 milestone May 19, 2021
@rtibbles rtibbles merged commit 5951a07 into learningequality:develop May 20, 2021
Copy link
Contributor

@indirectlylit indirectlylit left a comment

Choose a reason for hiding this comment

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

I think this PR may have taken away some single-quotes that were actually correct (cc @jonboiser @rtibbles)

message: "Sync '{facilityName}'",
message: 'Sync {facilityName}',
Copy link
Contributor

Choose a reason for hiding this comment

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

This change and similar ones looks incorrect to me. From the design system:

Always wrap user-generated text in 'single quotes'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

facilityName is interpolated to something like 'My facility' (a1b2), so the full message should match what you're saying. Check out the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, interesting – I didn't realize that facilityNameWithId was used inside other strings. I think that this pattern, while DRY, is technically incorrect for i18n strings because we're essentially using string concatenation to join two incomplete phrases.

A more technically correct string would include both inputs:

     message: `Sync '{facilityName}' ({id})`, 

My understanding is that this less-DRY format is easier for the translators to work with effectively.

(cc @radinamatic in case you have guidance and @jredrejo from the original change)

None of this is super critical – just wanted to flag for discussion and future work.

@jonboiser jonboiser deleted the update-facility-task-strings branch May 20, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) changelog Important user-facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

name of facility is incorrectly wrapped in single-quotes
3 participants