Skip to content
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

Sidepanel visibility on canvas focus #1173

Merged

Conversation

arthurian
Copy link
Contributor

When a canvas is focused, it takes 3 clicks on the TOC icon to open the side panel. This PR fixes that so that one click toggles the side panel open or closed.

Steps to reproduce:

  1. Go to http://projectmirador.org/demo/.
  2. In the first window on the left, click on the first canvas displayed in the bottom panel to focus it.
  3. Click the TOC icon to toggle the side panel. Expected result is that nothing will happen.
  4. Click again. Nothing should happen.
  5. Click again. Finally, the side panel should open.

Discussion:

The issue seems to be that most of the visibility logic is in Mirador.Window.sidePanelVisibility() but there's also a little bit in Mirador.SidePanel.render(), and those two can and do get out of sync. Based on how the code is currently written, it seemed to make the most sense to just get rid of the visibility logic in Mirador.SidePanel and simply let Mirador.Window handle that (at least for now -- maybe a later refactor will change that).

This change shouldn't impact the logic around rendering the panel open/closed initially, since sidePanelVisibility() is determined by settings.sidePanelVisible the first time it is called. The tab objects contained in the side panel appear to override that setting if they detect no content. Subsequent changes to visibility are carried through window updates as the value of window.sidePanelVisible is toggled.

@rsinghal @aeschylus Do you see any problems with this change?

@coveralls
Copy link

Coverage Status

Coverage increased (+1.1%) to 56.355% when pulling a9cdd42 on Harvard-ATG:bugfix/sidepanel-visibility into 01101de on ProjectMirador:2.1.2.

@aeschylus
Copy link
Collaborator

Hi Arti. Thanks for the catch and the thorough description and tests. Everything looks good to me. Down the line we can synchronise the state more explicitly, but it's good to have one way of doing it for now.

Thanks again.

@aeschylus aeschylus merged commit 3280dd3 into ProjectMirador:2.1.2 Nov 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants