Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Saira-A! See below for some questions and comments. I guess the biggest question is what our default should be. Should we just disable help everywhere for now? (Or should we enable it in just one extension while this PR is in review, so it's easy to test/compare, and then switch it off before merging?)
src/content-handlers/iiif/extensions/uv-aleph-extension/config/config.json
Outdated
Show resolved
Hide resolved
src/content-handlers/iiif/extensions/uv-model-viewer-extension/config/config.json
Outdated
Show resolved
Hide resolved
src/content-handlers/iiif/extensions/uv-openseadragon-extension/config/config.json
Show resolved
Hide resolved
src/content-handlers/iiif/modules/uv-shared-module/HeaderPanel.ts
Outdated
Show resolved
Hide resolved
src/content-handlers/iiif/extensions/uv-pdf-extension/config/config.json
Outdated
Show resolved
Hide resolved
Thanks @demiankatz! Yes, I was intending to have it disabled after it's been tested/ before it's merged. I got a linting error which I tried to fix but now I've got conflicts so I might just close this PR and redo it |
Looks like the conflicts are related to the autogenerated documentation. You should be able to resolve the conflicts by running |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Saira-A -- see below for a couple of interrelated comments.
| this.$rightOptions.append(this.$settingsButton); | ||
|
|
||
| this.$helpButton = $(` | ||
| <a class="btn imageBtn help" tabindex="0" title="${this.content.help}" role="button"> |
There was a problem hiding this comment.
If help is optional above, then I think we would need to have fallback logic here in case it's unset. I think it's preferable to make it mandatory, but maybe that has some annoying side effect that motivated your decision. Happy to discuss further as needed!
There was a problem hiding this comment.
If I make it mandatory, the error in OSD and PDF comes back unless I put it back to how it was before with the option repeated in their config files. Other options like settingsButtonEnabled are already repeated, so it would be the easier choice to just do the same, though it might not be the best practice. What do you think?
There was a problem hiding this comment.
I'm only talking about the HeaderPanelContent object's help string, which contains the translation for the button label. I think the other settings should remain optional as they presently stand... but it seems to me like this label either needs to be required, or it needs a fallback -- otherwise we could set the title to an undefined value when building the button.
There was a problem hiding this comment.
I've made it required now, I still had to repeat "help": "$help" in the content for the PDF and OSD config though to get rid of the error. I could add a fallback like this 'title="${this.content.help || 'Help'}' instead perhaps but of course that's not ideal for different languages.
@LanieOkorodudu also pointed out that I need to add it to mobile as well, which I'll do as a separate PR
There was a problem hiding this comment.
I don't mind having to repeat "help": "$help" too much, since the actual value of the string is still centralized, so customizing/changing it won't be too painful or confusing. Thanks!
There was a problem hiding this comment.
Great! I've broken the checks again so I'll just fix that before it's merged
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @Saira-A, this looks good to me now!
I'm setting this review to "request changes" simply as a reminder that we need to turn off the help button by default in the OSD extension before we merge this... but it's useful to leave it on for now so people can test it.
I have confirmed that the button displays and functions correctly in the OSD extension where it is turned on, and that it disappears in the PDF extension where it is turned off. So that all seems correct to me!
I'll now wait until a couple of other people have a chance to look this over. Once we have some other approvals, please revert the default setting and then I'll be happy to change my review to an approval and get this merged.
|
@mialondon, I am just wondering if the ''help'' button will also work on mobile view? Though it's looks nice when the UV is opened on a desktop. Looking at issue #1310 it is not clear if the help button functionality should only be available on desktop. |
|
@LanieOkorodudu, I believe @Saira-A plans to add a mobile help button, but wanted to get the basic functionality built first. I think that will be a separate follow-up PR. |
|
Looks good @Saira-A ! |
github-project-automation
bot
moved this from IN TESTING
to COMPLETED
in Community Sprint Feb 2025
4 participants
#1310
A new help icon has been added to the header panel, which when clicked opens up the webpage that has been set in the 'helpUrl' config option in a new tab. If the helpurl has been left null or the 'helpEnabled' option has been set to false then the help button does not show.