Add labels to gallery thumbnail size controls#1775
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@whitewooood is attempting to deploy a commit to the Universal Viewer Team on Vercel. A member of the Team first needs to authorize it. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @whitewooood -- I took a quick look and have a question and suggestion below.
| this._$sizeDownButton = $( | ||
| '<input class="btn btn-default size-down" type="button" value="-" />' | ||
| ); | ||
| ).attr("aria-label", this.options.data.content.decreaseSize); |
There was a problem hiding this comment.
Is there a particular reason why you chose to shorten the string names when including them in content? I think it might be better to use decreaseThumbnailSize or decreaseThumbSize instead of just decreaseSize to reduce potential ambiguity if we add more strings in the future relating to different size-related functionality. (This is also true of increaseSize and especially size).
| "$shareLink": "Dolen i'r dudalen hon", | ||
| "$shareInstructions": "Er mwyn rhannu'r eitem hon, cop\u00efwch y URL isod.", | ||
| "$size": "Maint:", | ||
| "$decreaseThumbnailSize": "Lleihau maint bawdluniau", |
There was a problem hiding this comment.
Where/how did you source your non-English translations? Do we need to have our translation team review them for accuracy?
|
Thanks for the review. I pushed an update using more specific content keys: For the non-English strings, I added direct equivalents so the new labels resolve in the existing locale files. They have not gone through a separate translation-team review, so adjustments from the translation workflow would be welcome. |
|
Thanks, @whitewooood, I have put this in the "in testing" column of the Community Board so we can discuss it at a future Community Call and figure out the best way to get all the translations reviewed. |
| this.options.data.content = Object.assign( | ||
| {}, | ||
| this.data().content, | ||
| this.options.data.content | ||
| ); |
There was a problem hiding this comment.
Is this necessary? If so, why?
(It's been a while since I've reviewed the config loading code here, which I realize is rather confusing -- but this doesn't seem to match patterns I've seen elsewhere, so I'm just curious about it).
There was a problem hiding this comment.
Thanks for pointing this out. I removed that constructor-level content merge in the latest commit.
I had added it as a fallback for the new labels, but it was not a good fit for the existing config-loading pattern. The component now applies the size-control labels from its current data in _init() and again in set(), which keeps the normal config flow intact and also lets the localized content supplied later by GalleryView.databind() update those labels.
Summary
Fixes #1488.
Validation