universalviewer

universalviewer copied to clipboard

UV version:

 [email protected]

I'm submitting a:

• [x] bug report => please fork one of these codesandbox examples with a repro of your issue and include a link to it below
• [ ] feature request => please use the user stories repo
• [ ] support request => Please do not submit support requests here, use stackoverflow

Current behavior:

When I use configuration option footerPanel.options.minimiseButtons to false, the text is not appearing.

Expected behavior:

Button text should appear in the footer panel when footerPanel.options.minimiseButtons is set to false

Notes:

I see that the following HTML

<button class="download btn imageBtn" title="Download" id="download-btn">
            <i class="uv-icon uv-icon-download" aria-hidden="true"></i>
            <span class="sr-only">Download</span>
</button>

uses class sr-only. That is responsible for hiding that text block.

Apparently this is set always: https://github.com/UniversalViewer/universalviewer/blob/dev/src/content-handlers/iiif/modules/uv-shared-module/FooterPanel.ts#L96

Isn't class miniseButtons in footerPanel not responsible for hiding button texts?

Looks like the problem was introduced in this commit, about four years ago:

https://github.com/UniversalViewer/universalviewer/commit/bf9f07317a71a80e05cfd6c68fc8b524c0766805

This looks like an effort to normalize the rendering of all buttons, but it seems to have extended the sr-only style to places where it did not previously exist.

I think it may make sense to keep the standardized button rendering, but maybe we can simply change the updateMinimisedButtons method to remove the sr-only class when appropriate instead of adding the minimiseButtons class... and then we might be able to eliminate the minimiseButtons class entirely.

@demiankatz it is not just that configuration option. I tested the following configuration options for example:

• modules.headerPanel.options.centerOptionsEnabled has no effect (uses defaulttrue)
• modules.headerPanel.options.settingsButtonEnabled has no effect (uses default true)

Did my proposed change help with the minimiseButtons setting? I'm wondering if these broken setting problems are all related, or if perhaps each needs an independent solution. (I haven't had time to dive back into the code yet, but let me know if further investigation would be helpful, and I can take a closer look when time permits).

@demiankatz no not really. None of the mentioned configuration settings have any effect.

For reference: in v4.0.25 the settings have an effect, so the "change" must have been introduced since then (and so only applies to the current dev branch)

@nicolasfranck, is it worthwhile (and do you have an appropriate setup to allow you) to run a git-bisect to figure out exactly where the break occurs? At the moment, the main branch is ahead of v4.0.25, so it's a little hard to tell exactly what is going on. Hopefully as we get our branching strategies pinned down a bit more, it will be easier to investigate this type of problem in the future... but in the meanwhile, a bisect might get right to the heart of the problem.

@demiankatz the main branch is some commits ahead, but those commits are more about adding readme sections, do not contain anything code related (right?).

I did do a git-diff, but I don't know what I am looking at.

@nicolasfranck, when I run git diff v4.0.25 main --name-only it looks like quite a few src files were changed between the 4.0.25 release and the current main branch; it's not just readme and documentation stuff. I think some work related to @edsilv's typing of the configurations is in play here, and that seems likely to be related to the problem somehow. I just thought a git bisect might be a good way to drill down exactly which commit is to blame, if there's an easy way to iterate testing the problem on builds of the project.

In any case, I'm putting this issue into the queue on the Universal Viewer Community Board to be sure it gets some attention.

ah, I see now a merge of the dev branch into the main 8 hours ago.

@nicolasfranck, it's been a few months; just wanted to follow up and see if there's still a mystery in need of solving here. If there's an easy way to reproduce the problem, I don't mind trying to set up a git bisect to narrow down where things went wrong, if that would be any help.

@demiankatz that would be helpful. As I said, even if I run it, I do not know what I am looking at.

@nicolasfranck, I just opened #1061 to implement the solution I proposed back on January 18th. It seems to work as expected for me. It would be great to write some tests to prevent regressions here, but I'm not sure if there's currently a way to manipulate configuration through the test suite. That's something we ought to figure out at some point!

I didn't have time to look into the other headerPanel options that you reported as non-working in subsequent comments. I thought it might make the most sense to solve the specific issue reported here, and if those other problems remain unresolved, perhaps we should open separate issues to address them independently.