Skip to content

Remove hard-coded text from infobox label.#1134

Merged
demiankatz merged 2 commits into
UniversalViewer:devfrom
FalveyLibraryTechnology:fix-infobox-label
Oct 22, 2024
Merged

Remove hard-coded text from infobox label.#1134
demiankatz merged 2 commits into
UniversalViewer:devfrom
FalveyLibraryTechnology:fix-infobox-label

Conversation

@demiankatz

Copy link
Copy Markdown
Contributor

This PR fixes a problem that was observed while working on #1083.

I'm pretty confident that the fix is correct, but I am unsure exactly how to trigger an infobox pop-up to test it; it appears to have something to do with the authentication flow.

@vercel

vercel Bot commented Oct 17, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 22, 2024 1:00pm

@jamesmisson jamesmisson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code looks good to me. I've asked in the IIIF Slack if anyone has any manifests that will trigger authentication.

$closeButton.attr("title", this.content.close);
$closeButton.on("click", (e) => {
e.preventDefault();
this.extensionHost.publish(IIIFEvents.HIDE_INFORMATION);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@demiankatz, just a quick question—would changing the event from "Hide" to "Show" help trigger the infobox pop-up for testing? Another thing to check would be if there's a manifest that requires authentication. Clicking the "More" button in that scenario should trigger the infobox to appear as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried something similar to that but had trouble getting it to work. I can probably try again tomorrow when I have a bit more time, though, if nobody can share with us a manifest to trigger the message in the meantime!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I looked again. The problem here is that triggering the SHOW_INFORMATION event requires an IExternalResource object, which is something to do with the authentication system. I'm not sure how to create or simulate one of these outside of the actual authentication workflow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I figured out a way! I created a showFakeInformation() method that's a copy of showInformation() with the auth-specific logic replaced with a hard-coded "hello world" and I wired that method up to get called when I click on the settings button. Here's a patch of my hack:

diff --git a/src/content-handlers/iiif/modules/uv-shared-module/HeaderPanel.ts b/src/content-handlers/iiif/modules/uv-shared-module/HeaderPanel.ts
index bcabeb39..355b7db6 100644
--- a/src/content-handlers/iiif/modules/uv-shared-module/HeaderPanel.ts
+++ b/src/content-handlers/iiif/modules/uv-shared-module/HeaderPanel.ts
@@ -93,6 +93,7 @@ export class HeaderPanel<
     });
 
     this.$settingsButton.onPressed(() => {
+      this.showFakeInformation();
       this.extensionHost.publish(
         IIIFEvents.SHOW_SETTINGS_DIALOGUE,
         this.$settingsButton
@@ -146,6 +147,23 @@ export class HeaderPanel<
     return false;
   }
 
+  showFakeInformation(): void {
+    var $message = this.$informationBox.find(".message");
+    $message
+      .html("hello world")
+      .find("a")
+      .attr("target", "_top");
+    var $actions = this.$informationBox.find(".actions");
+    $actions.empty();
+
+    this.extensionHost.publish(IIIFEvents.MESSAGE_DISPLAYED, this.information);
+
+    this.$informationBox.attr("aria-hidden", "false");
+    this.$informationBox.show();
+    this.$element.addClass("showInformation");
+    this.extension.resize();
+  }
+
   showInformation(args: InformationArgs): void {
     const informationFactory: InformationFactory = new InformationFactory(
       this.extension

With that in place, I was able to confirm that the close button in the info box includes an appropriately translated aria-label, as evidenced by this screen shot showing the Welsh translation:

VirtualBox_VuFind (Villanova DigLib) Development Box_22_10_2024_09_08_51

I think this is the best we are going to be able to do testing-wise without a real demonstration manifest. If that's enough for you to be satisfied, please approve and I'll get this merged!

@demiankatz demiankatz merged commit 1685b15 into UniversalViewer:dev Oct 22, 2024
@demiankatz demiankatz deleted the fix-infobox-label branch October 22, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Community Sprint COMPLETED

Development

Successfully merging this pull request may close these issues.

3 participants