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

SF-2710 Clean up i18n of book-multi-select #2430

Merged
merged 1 commit into from
May 2, 2024

Conversation

Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Apr 24, 2024

This change is Reviewable

@Nateowami Nateowami added the will require testing PR should not be merged until testers confirm testing is complete label Apr 24, 2024
@pmachapman pmachapman self-assigned this Apr 28, 2024
@pmachapman pmachapman self-requested a review April 28, 2024 13:39
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm: The change looks good. This is the first time I have noticed the transloco pipe - it looks like it is more efficient in this case due to using change detection, if I understand the docs correctly?

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

@pmachapman Maybe? I didn't use it for efficiency; I used it because it allowed me to not use the book_select namespace throughout the rest of the file. I would have used the I18nService, but adding that to the component requires changes to the tests, and it wasn't a trivial change, unfortunately.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed will require testing PR should not be merged until testers confirm testing is complete ready to test labels Apr 29, 2024
@Nateowami Nateowami force-pushed the fix/SF-2710-clean-up-book-multi-select branch from 95ebb22 to ab40a1d Compare May 2, 2024 14:57
@Nateowami Nateowami enabled auto-merge (squash) May 2, 2024 14:57
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.05%. Comparing base (771a349) to head (ab40a1d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2430   +/-   ##
=======================================
  Coverage   77.05%   77.05%           
=======================================
  Files         513      513           
  Lines       29434    29434           
  Branches     4789     4789           
=======================================
  Hits        22679    22679           
  Misses       6008     6008           
  Partials      747      747           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Nateowami Nateowami merged commit e12d2df into master May 2, 2024
16 checks passed
@Nateowami Nateowami deleted the fix/SF-2710-clean-up-book-multi-select branch May 2, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing complete Testing of PR is complete and should no longer hold up merging of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants