-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
minimiseButtons
configuration option no longer works as intended
#952
Comments
Looks like the problem was introduced in this commit, about four years ago: 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:
|
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 |
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 |
@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. |
UV version:
I'm submitting a:
Current behavior:
When I use configuration option
footerPanel.options.minimiseButtons
tofalse
,the text is not appearing.
Expected behavior:
Button text should appear in the footer panel when
footerPanel.options.minimiseButtons
is set tofalse
Notes:
I see that the following HTML
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
infooterPanel
not responsible for hiding button texts?The text was updated successfully, but these errors were encountered: