-
Notifications
You must be signed in to change notification settings - Fork 58
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
Improve sidebar initialization #555
Conversation
Apply click and transition end event listeners directly to the elements that need them instead of applying them to the document and relying on event delegation. We also now use vanilla JS to add and remove event listeners instead of jQuery, and we remove the event listeners if the layouts are removed the page.
arrive is a lot lighter and can be included in each component directly
Improves the transition animation
And init all sidebars at once
@cpsievert This is ready for final review. Also note that this PR pairs with rstudio/shinycoreci#164, which includes tests around the sidebar behavior covered here. |
Looking good overall. It's up to you whether you want to investigate how a web component might improve/simplify things. Please also update your original comment to better reflect what this PR does, thanks! |
Was it intentional to remove the data attribute in 0cb6d58? |
Nope, thanks for catching that! I tried applying the suggestion via vscode and it bungled it 😬 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up @schloerke, we'll probably want to roll these changes into PyShiny (although, it might make sense to wait until #557 is also done)
The core goal of this PR is to ensure that sidebar layouts are correctly initialized regardless of how they are loaded. There are essentially three methods of inclusion to consider:
insertUI()
orrenderUI()
.In the final version, we use a
<script>
tag (because it's easy and available everywhere) paired with a static methodSidebar.initCollapsibleAll()
that initializes all collapsible sidebars on the page at once.This PR also moves from using the
Sidebar
class with only static methods as a way to collect sidebar-related functions to actually creating aSidebar
instance for each initialized sidebar. There are only two public methods on this class,.isClosed
(a getter) and.toggle()
, which accepts"open",
"close", or
"toggle"(implied when
.toggle()is called without arguments). These are used in the
.getValue()and
.receiveMessage()methods of the Shiny
SidebarInputBinding`. Most of the changes in this PR are around distinguishing between static and instance methods.Another small fix in this PR: we better coordinate the timing of the collapse toggle icon with the sidebar collapse transition itself to ensure that final state event listener fires (on the toggle icon transitionend) at the same time that the sidebar is fully collapsed.
Historical notes:
I tried a few ideas and have kept my exploration in tact in this PR, but the goal is the same and the approach is relatively similar:
My first attempt used mutation-summary, which is well-designed but ultimately quite heavy as a library. I then tried arrive.js, which is smaller and even easier to use but still a little too heavy to bundle directly into our minimized component JavaScript. The final approach in this PR is a custom class,
DocumentObserver
, built aroundMutationObserver
, that does exactly what we need while building directly on browser APIs. All three versions achieve the same result, but the hand-crafted approach usingDocumentObserver
adds only about 1kB to the minified sidebar JavaScript.While working on the dynamic UI, I reconfigured our event listeners to be better contained within the
Sidebar
class. I think this improves readability and it makes it allows us to directly attach the event listeners to targeted elements without having to rely on event bubbling. It also lets us remove unused event listeners when sidebar layouts are removed from the DOM.It's worth pointing out that
sidebar.ts
is now just aSidebar
class and theSidebarInputBinding
; theDocumentObserver
design allows us to set up a private static object on theSidebar
class that handles onload and DOM observing callbacks. It also means that we only need to include the sidebar html_dependency, without needing any initialization code in the markup, and we can have a strong guarantee that any existing or new sidebars will be initialized when added to the page.Test App
The above test app was used as the basis for rstudio/shinycoreci#164