PKPD-30 collapsible sidebar #568
Conversation
… and simulations sidebar
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## ui-enhancements #568 +/- ##
================================================
Coverage 76.17% 76.17%
================================================
Files 108 108
Lines 5600 5600
================================================
Hits 4266 4266
Misses 1334 1334 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| const element = document.getElementById("main-content"); | ||
| element?.classList.remove("on-expand"); | ||
| element?.classList.add("on-collapse"); | ||
|
|
||
| const simulationsNav = document.getElementById("simulations-content"); | ||
| if (simulationsNav) { | ||
| simulationsNav?.classList.remove("on-expand"); | ||
| simulationsNav?.classList.add("on-collapse"); | ||
| } |
There was a problem hiding this comment.
This logic could maybe move into a function outside of the component, to make it clear that the DOM is being modified outside of React.
Also, the DOM changes here won't be tracked by React's virtual DOM, so I'm not 100% sure if they'll persist across re-renders.
| const element = document.getElementById("main-content"); | ||
| element?.classList.remove("on-collapse"); | ||
| element?.classList.add("on-expand"); | ||
|
|
||
| const simulationsNav = document.getElementById("simulations-content"); | ||
| if (simulationsNav) { | ||
| simulationsNav?.classList.remove("on-collapse"); | ||
| simulationsNav?.classList.add("on-expand"); | ||
| } |
There was a problem hiding this comment.
Same comments here about modifying the DOM outside of the React virtual DOM.
| sm: isExpanded ? drawerExpandedWidth : drawerCollapsedWidth, | ||
| }, | ||
| flexShrink: { sm: 0 }, | ||
| }} | ||
| aria-label="mailbox folders" |
There was a problem hiding this comment.
Not related to this PR, but I just noticed that these ARIA labels are wrong.
| flexShrink: { sm: 0 }, | ||
| height: "100vh", | ||
| backgroundColor: '#FBFBFA', | ||
| borderRight: '1px solid #DBD6D1' | ||
| }} | ||
| aria-label="mailbox folders" |
There was a problem hiding this comment.
Also not related to this PR, but the wrong ARIA label here too.
| /> | ||
| )} | ||
| <Box | ||
| component="main" | ||
| id="main-content" |
There was a problem hiding this comment.
I'd be inclined to set the expanded/collapsed className here, rather than modify the DOM directly.
|
@eatyourgreens Persistence was not an issue, as those classes are only controlling the animations, however, I fully agree with your point that modifying the DOM directly is not the best solution. I have refactored the whole solution so that those classes are not applied directly to DOM. I've also added a context to easily have access to those classes in case it's needed in the future. I've also updated those aria-labels that you have mentioned, and on that note, added additional aria-labels to buttons controlling the collapsing/expanding |
|
@wniestroj Changes look good to me. I've one small comment about avoiding generic labels for landmark elements like By the way, when I collapse the sidebar for Simulations, there's a lot of whitespace on the right. The plots don't fill the available width. Screen.Recording.2024-11-21.at.08.22.55.mov |
|
Apologies, I tested this on the Simulations page, but when I look at other views, the animation for the main content panel isn't working quite correctly. I'm not sure if this broke when I merged in the latest Screen.Recording.2024-11-21.at.11.10.47.mov |
update aria-label to more specific label Co-authored-by: Jim O'Donnell <[email protected]>
# Conflicts: # frontend-v2/src/features/main/Sidebar.tsx
…to PKPDX-29 # Conflicts: # frontend-v2/src/features/main/Sidebar.tsx
…ndow.resize event; added additional plot widths
|
@eatyourgreens I wasn't able to listen to your recording on that point about screen readers, as it was uploaded without sound, however I have tested it myself and I think I understood your point. At first I have applied your proposed change to aria-label to 'project navigation' however, that also caused word 'navigation' be repeated (project navigation navigation), therefore I've decided to change the aria-label to 'main' so that this element is voiced as 'main navigation'. Feel free to propose other labels and let me know if you would have any additional points regarding the screen readers Regarding first video of a lot of white space on simulations page - this recorded view seems odd, I'm assuming you might've been resizing the window, which didn't update the plot widths. I have added two things:
Regarding the second video, I have tested it throughly before creating a merge request and never had such issue. I haven't tested the code after your merge of ui-enchancements to the branch, as somehow automatically I have merged the ui-enchancements branch to this branch and tested it again - and I haven't been able to replicate this issue as well. I've pushed both mentioned changes, as well as merge of the ui-ench... branch. Could you check if this issue is still occurring? And if so, could you provide a path to recreating this issue? |
|
@wniestroj I've pulled your latest changes and made another screen recording. The bug is still there, but it is intermittent. The animation doesn't always run on either opening or closing the sidebar. This is recorded in Firefox 132. Screen.Recording.2024-11-21.at.12.51.37.mov |
|
Re. the simulations view, I hadn't resized the window but here's a recording where I enlarge the window from very narrow to very wide. The plot starts off overflowing the viewport width, then shrinks (with white space to the right), then switches to a two column grid. Screen.Recording.2024-11-21.at.13.05.06.mov |
|
I have refactored the way the widths of the plots, dropping breakpoints solution and calculating width of plots in dependence to parent width (not pushed yet), however, I'm unable to find a solution to those animations not being triggered some of the times. I'll get back to it first thing tomorrow, but I'm afraid I might need to drop the animations entirely if I'm unable to make it work |
|
If it's only an intermittent bug in Firefox, then it might not be worth spending a lot of time debugging. |
…on sidebar collapsible sections to be collapsed by default; Dislpay Parameter input below the slider
|
|
|
@eatyourgreens I have updated how plot widths are calcualted, so that they should always take up all available space. Note: I have added a half a second debounce for performance issues, so that it does not enter an infinite loop of resizing, therefore continuous resizing will not trigger the resizing of plots, only .5s after the resizing the widths will get updated. Regarding your issue of animation not always triggering, after alignment with MIchael, Martin and Stefanie, we have decided not to pursue a fix for that, as it seems to be working fine on Chrome, and within our organization we are forced to use Chrome (we are unable to even download Firefox). I have also raised concerns about some performance issues of Navigation animation - I'm not sure is those are caused by my machine being constantly overloaded, or if those issues are caused by something else. We have decided to go ahead and push my changes to the environment and in case those issues are still present after the deployment, we will either get rid of the animations as a whole, or remove animations from the sidebar |
|
As @wniestroj mentioned, we'll merge this in for now so I can deploy on the test server. We can turn off the animations later on if they become an issue. |
|
Sounds good to me. One improvement that I would suggest: remember the expanded/contracted state of the sidebar across sessions (maybe in a cookie or local storage.) At the moment, the sidebar is always expanded when I reload the page or open a new tab. |
Added collapsible sideber, added animations to main content and simulations sidebar