Make Persistent Menu Navigation a Sidebar #35142

AddisonDunn · 2024-09-18T20:08:14Z

Product Description

Jira: one and two.

This PR:

Turns the persistent menu navigation into a sidebar when on desktop, but remaining an overlay on mobile
To accomplish the bullet above, I needed to "lock" the main content container, so that scrolling happens on the content container instead of the entire page. This way the persistent menu and the other content can be separately scrollable.
Removes the data preview bar on mobile
Removes the gray background around forms on mobile

To see what I mean by "locked" and "separately scrollable", see this video:
https://github.com/user-attachments/assets/eed7edaf-1036-4fb0-a705-16e4f3847a57

Gray background removed on mobile:

Technical Summary

I made some key changes to margins and the underlying DOM layout of web apps that I am hoping will be more of a boon than a burden.

Safety Assurance

Safety story

Everything looks good with testing locally: main app selection screen, case list screen with case tiles and map, form screen
This will go through QA

QA Plan

Going to go through QA, ticket TBD

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

… mobile

and so that the sidebar persistent menu and main content container can be seperately scrollable, similar to how they are in Jira

nospame

Did you consider leaving the persistent nav as a regular Bootstrap offcanvas element and explicitly pushing over the main content on larger screens? Bootstrap provides options for the offcanvas both to not darken the background and to allow scrolling under it that could help make it possible. (demo and options)

The current implementation seems like it will probably work, but it's a pretty good chunk of code that might be mostly doable with built-in Bootstrap functionality.

nospame · 2024-09-30T21:55:42Z

corehq/apps/cloudcare/templates/cloudcare/partials/debugger.html

+        },
+        visible: !smallScreenIsEnabled(),


Could this bit be done with Bootstrap using classes on the div instead? Something like:

<div id="instance-xml-home" class="debugger d-none d-lg-block"

Based on the examples here: https://getbootstrap.com/docs/5.0/utilities/display/#hiding-elements

nospame · 2024-09-30T22:01:04Z

corehq/apps/cloudcare/templates/cloudcare/partials/form_entry/form.html

@@ -47,7 +47,7 @@ <h1 class="title" data-bind="text: title, visible: !showInFormNavigation()"></h1
              visible: showSubmitButton,
              css: { 'sticky-submit': isAnchoredSubmitStyle },
              style: {
-                'bottom':isAnchoredSubmitStyle && {{ request.couch_user.can_edit_data|yesno:'true,false' }} {% if environment != "web-apps" %} && false {% endif %} ? '30px' : '0' {# data preview bar #}
+                'bottom':isAnchoredSubmitStyle && !isAppPreview && !smallScreenIsEnabled && {{ request.couch_user.can_edit_data|yesno:'true,false' }}? '30px' : '0' {# data preview bar #}


It would be nice to tie this to a single variable that gets at the point, if possible. So instead of all this logic the conditional would look something like isAnchoredSubmitStyle && showDataPreviewBar ?

I faced a similar problem in #35069

The issue there is that breadcrumbs may or may not be present, and if they are, other elements need to be offset accordingly. What I ended up doing was having the breadcrumbs component set a has-breadcrumbs class on #cloudcare-main:

https://github.com/dimagi/commcare-hq/pull/35069/files#diff-59204083f0efa0e7d40586e28666900ebf9970de0550b5ffad1cd9f8fbc2c918R1329-R1335

Then I can offset other elements with just CSS without needing to replicate the set of circumstances that indicate whether that element is present:

https://github.com/dimagi/commcare-hq/pull/35069/files#diff-ff9b57d6462c3e8f90a3b6dbb35f575bd199ba487691cbeeca5a1b0d686dc986R7-R9

I was really hoping I'd be able to find a JS-free solution to that problem - where the element just takes up space on the page and automatically offsets the other element, based on the DOM structure. Couldn't find anything online that made it sound like that was possible with position: sticky; though.

Another perk of that approach is that this line could be replaced with SCSS, and you could use a reference to $minimized-debugger-height instead of a hardcoded 30px

It would be nice to tie this to a single variable that gets at the point, if possible. So instead of all this logic the conditional would look something like isAnchoredSubmitStyle && showDataPreviewBar ?

Yeah I agree, it's messy. My rationale for not doing what you suggested was that I'd have to import initial page data on the JS side to access the couch user data (I'm guessing you can access couch user data in JS?) so the messiness would just be in JS instead of HTML.

nospame · 2024-09-30T22:08:31Z

corehq/apps/cloudcare/templates/cloudcare/formplayer_home.html

+        <div id="sidebar-region" class="noprint-sub-container mt-3"></div>
+        <div id="content-container" class="container mt-3">


This doesn't seem equivalent to the margin removed from .case-tile-container, which I as far as I can tell would only have affected a single div as previously written (the <div id="persistent-case-tile" class="print-container">). Is that intentional?

Also noting that the separate print bottom margin for case-tile-container would probably give different results with this change, since the margin is applied differently.

commcare-hq/corehq/apps/hqwebapp/static/cloudcare/scss/formplayer-webapp/case-tile.scss

Line 115 in 380386f

margin-bottom: 0px;

nospame · 2024-09-30T22:16:31Z

corehq/apps/hqwebapp/static/cloudcare/scss/formplayer-webapp-main.scss

@@ -11,6 +11,8 @@ formplayer, please make changes to the cloudcare styles.
 **/

 $breadcrumb-height-cloudcare: 36px;
+$cloudcare-nav-height: 30px;


nit: know you didn't name this one, but it seems like we'd want to follow convention with $breadcrumb-height-cloudcare and call it $nav-height-cloudcare (or rename the breadcrumb variable if that seems better). Maybe a similar change for $minimized-debugger-height.

nospame · 2024-09-30T22:18:19Z

corehq/apps/cloudcare/templates/cloudcare/partials/menu/persistent_menu.html

+  <a id="persistent-menu-arrow-toggle"
+     type="button"


nit: could this just be a button element instead?

nospame · 2024-09-30T22:58:04Z

corehq/apps/hqwebapp/static/cloudcare/scss/formplayer-webapp/content.scss

@@ -42,6 +42,12 @@ body {
  height: calc(100vh - #{$cloudcare-nav-height} - $breadcrumb-height-cloudcare);
 }

+#sidebar-and-content.remove-padding-on-mobile {


I think the class would be more accurately named remove-margins-on-mobile

AddisonDunn · 2024-10-01T14:54:25Z

Did you consider leaving the persistent nav as a regular Bootstrap offcanvas element and explicitly pushing over the main content on larger screens?

@nospame "Pushing over the main content" is the approach currently taken in the codebase for the SSCS sidebar. Ethan proposes shifting from the more manual "push" approach to a flexbox approach instead in this PR (applying flex grow to the main content div, so that it naturally expands/contracts in response to changes in width of other elements in the same row). This PR sort of doubles-down on that idea, by making the persistent menu part of that flexbox layout instead of being an overlay. Since the desire of the request is to make the sidebar by default shown and to "take up space" in the DOM, this seemed best to me, instead of giving the sidebar an absolute position and "pushing the content over" like we were previously doing for the SSCS sidebar. Thus the choice of collapse over offcanvas, which seems more suitable for something that takes up space in the DOM.

esoergel

Left a bunch of comments, mostly raising for discussion since I'm not certain how best to do a lot of this stuff - please don't take those as meaning "you must fix this."

esoergel · 2024-10-01T13:42:26Z

corehq/apps/cloudcare/templates/cloudcare/partials/debugger.html

@@ -7,7 +7,8 @@
            'debugger-minimized': isMinimized,
            'debugger-maximized': !isMinimized(),
            'debugger-updating': updating
-        }
+        },
+        visible: !smallScreenIsEnabled(),


I think this is doable with bootstrap classes rather than needing JS:
https://getbootstrap.com/docs/5.0/utilities/display/#hiding-elements
Like, if you add d-none d-lg-block to the class a few lines up, I believe that will behave the same.

esoergel · 2024-10-01T13:45:18Z

corehq/apps/cloudcare/templates/cloudcare/partials/form_entry/form.html

@@ -47,7 +47,7 @@ <h1 class="title" data-bind="text: title, visible: !showInFormNavigation()"></h1
              visible: showSubmitButton,
              css: { 'sticky-submit': isAnchoredSubmitStyle },
              style: {
-                'bottom':isAnchoredSubmitStyle && {{ request.couch_user.can_edit_data|yesno:'true,false' }} {% if environment != "web-apps" %} && false {% endif %} ? '30px' : '0' {# data preview bar #}
+                'bottom':isAnchoredSubmitStyle && !isAppPreview && !smallScreenIsEnabled && {{ request.couch_user.can_edit_data|yesno:'true,false' }}? '30px' : '0' {# data preview bar #}


So this style offsets the submit button if the data preview bar is present? That seems brittle (I know you didn't add it) - any idea how tricky it'd be to structure the DOM such that this is unnecessary?

esoergel · 2024-10-01T13:56:31Z

corehq/apps/cloudcare/templates/cloudcare/formplayer_home.html

@@ -82,7 +82,7 @@

  <div id="cloudcare-main" class="cloudcare-home-content">
    <section id="cases"></section>
-    <div id="menu-container">
+    <div id="main-container">


esoergel · 2024-10-01T14:51:37Z

corehq/apps/cloudcare/templates/cloudcare/partials/form_entry/form.html

@@ -47,7 +47,7 @@ <h1 class="title" data-bind="text: title, visible: !showInFormNavigation()"></h1
              visible: showSubmitButton,
              css: { 'sticky-submit': isAnchoredSubmitStyle },
              style: {
-                'bottom':isAnchoredSubmitStyle && {{ request.couch_user.can_edit_data|yesno:'true,false' }} {% if environment != "web-apps" %} && false {% endif %} ? '30px' : '0' {# data preview bar #}
+                'bottom':isAnchoredSubmitStyle && !isAppPreview && !smallScreenIsEnabled && {{ request.couch_user.can_edit_data|yesno:'true,false' }}? '30px' : '0' {# data preview bar #}


I faced a similar problem in #35069

The issue there is that breadcrumbs may or may not be present, and if they are, other elements need to be offset accordingly. What I ended up doing was having the breadcrumbs component set a has-breadcrumbs class on #cloudcare-main:

https://github.com/dimagi/commcare-hq/pull/35069/files#diff-59204083f0efa0e7d40586e28666900ebf9970de0550b5ffad1cd9f8fbc2c918R1329-R1335

Then I can offset other elements with just CSS without needing to replicate the set of circumstances that indicate whether that element is present:

https://github.com/dimagi/commcare-hq/pull/35069/files#diff-ff9b57d6462c3e8f90a3b6dbb35f575bd199ba487691cbeeca5a1b0d686dc986R7-R9

I was really hoping I'd be able to find a JS-free solution to that problem - where the element just takes up space on the page and automatically offsets the other element, based on the DOM structure. Couldn't find anything online that made it sound like that was possible with position: sticky; though.

Another perk of that approach is that this line could be replaced with SCSS, and you could use a reference to $minimized-debugger-height instead of a hardcoded 30px

esoergel · 2024-10-01T15:01:45Z

corehq/apps/cloudcare/templates/cloudcare/partials/menu/persistent_menu.html

-    <div class="offcanvas-header persistent-menu-max-width d-flex align-items-center justify-content-between">
+  <div id="persistent-menu-container" class="bg-white h-100 position-relative overflow-scroll"
+    aria-labelledby="persistent-menu-label">
+    <button type="button" id="close-button" class="btn-close text-reset position-absolute"


#close-button might be too generic a term, risking conflict with other elements on the page

esoergel · 2024-10-01T15:19:55Z

corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views.js

+        onDomRefresh: function () {
+            this.handleSmallScreenChange(cloudcareUtils.smallScreenIsEnabled());


out of curiosity, why is this in onDomRefresh rather than onRender or initialize?

I don't think the HTML elements can be referenced using jQuery in onRender or initialize, at least that was the case with my testing. But actually onAttach might be more appropriate, being just earlier in the lifecycle, and I see that it being used in such a way elsewhere in the file.

corehq/apps/cloudcare/templates/cloudcare/formplayer_home.html

esoergel · 2024-10-01T15:28:12Z

corehq/apps/cloudcare/templates/cloudcare/formplayer_home.html

      <section id="cloudcare-notifications" class="container notifications-container"></section>
      <div class="container case-tile-container">
        <div id="persistent-case-tile" class="print-container"></div>
      </div>
-      <div class="d-lg-flex">
-        <div id="sidebar-region" class="noprint-sub-container mt-3"></div>
-        <div id="content-container" class="container mt-3">


this container is necessary to prevent wide elements from overflowing the div. That was the issue addressed by 177ccc7

I see you put a max-width on the element, which should probably address that, though does that also work well on smaller screen sizes?

esoergel · 2024-10-01T15:39:28Z

corehq/apps/cloudcare/static/cloudcare/js/formplayer/app.js

@@ -306,6 +307,7 @@ hqDefine("cloudcare/js/formplayer/app", [
        var sess = WebFormSession.WebFormSession(data);
        sess.renderFormXml(data, $('#webforms'));
        $('.menu-scrollable-container').addClass("d-none");
+        $('#sidebar-and-content').addClass('remove-padding-on-mobile');


Does this really have to apply only to form entry? Wondering if we could instead remove the padding on mobile for everything. Or if say menus do need padding, then maybe that could be part of the menu definition.

esoergel · 2024-10-01T15:41:37Z

corehq/apps/cloudcare/static/cloudcare/js/formplayer/menus/views.js

@@ -1561,6 +1561,9 @@ hqDefine("cloudcare/js/formplayer/menus/views", [
            } else {
                $('#persistent-menu-container').removeClass(containerMobileClasses);
                $('#persistent-menu-container').addClass(containerDesktopClasses);
+                if (sessionStorage.showPersistentMenu !== 'false') {


nospame · 2024-10-01T17:49:48Z

This PR sort of doubles-down on that idea, by making the persistent menu part of that flexbox layout instead of being an overlay. Since the desire of the request is to make the sidebar by default shown and to "take up space" in the DOM, this seemed best to me, instead of giving the sidebar an absolute position and "pushing the content over" like we were previously doing for the SSCS sidebar.

This makes sense within the existing context, but I think it's worth considering/remembering that Bootstrap is really explicitly mobile-first. So by designing this as a web-first feature, this approach is basically working against Bootstrap instead of using it to our advantage. I don't know enough about the particular feature to say if I think it's the wrong way to go about things here, but I think it's important to have the discussion about whether it is the right approach (and maybe USH devs have already done that and I'm late to the party).

AddisonDunn requested a review from esoergel September 18, 2024 20:08

dimagimon added Risk: High Change affects files that have been flagged as high risk. Risk: Medium Change affects files that have been flagged as medium risk. labels Sep 18, 2024

AddisonDunn added 8 commits September 18, 2024 16:29

remove data preview bar on mobile

a0ae928

move content padding to more intentional place

55be2ef

rename main container

842ad5c

make toolbar heights accessible variables

7e2b3c1

make persistent menu nav a sidebar on desktop but still an overlay on…

fa979a8

… mobile

reorganize main layout so that main content container is 'locked'

4319ec4

and so that the sidebar persistent menu and main content container can be seperately scrollable, similar to how they are in Jira

make breadcrumb bottom shadow a border instead

a78a365

remove form margins on mobile

919ed4f

AddisonDunn force-pushed the ad/open-persistent-menu branch from c4bdfa3 to 919ed4f Compare September 18, 2024 20:33

dimagimon removed the Risk: High Change affects files that have been flagged as high risk. label Sep 18, 2024

keep menu closed between pages if user closed it

e775d4f

AddisonDunn marked this pull request as ready for review September 30, 2024 14:48

AddisonDunn requested review from orangejenny, biyeun, MartinRiese, stephherbers, Robert-Costello, Jtang-1 and nospame as code owners September 30, 2024 14:48

AddisonDunn mentioned this pull request Sep 30, 2024

Various web apps responsive UI changes #35095

Closed

3 tasks

nospame reviewed Sep 30, 2024

View reviewed changes

esoergel reviewed Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Persistent Menu Navigation a Sidebar #35142

Make Persistent Menu Navigation a Sidebar #35142

AddisonDunn commented Sep 18, 2024

nospame left a comment •

edited

Loading

nospame Sep 30, 2024

nospame Sep 30, 2024

esoergel Oct 1, 2024

AddisonDunn Oct 1, 2024

nospame Sep 30, 2024

nospame Sep 30, 2024

nospame Sep 30, 2024

nospame Sep 30, 2024

AddisonDunn commented Oct 1, 2024

esoergel left a comment

esoergel Oct 1, 2024

esoergel Oct 1, 2024

esoergel Oct 1, 2024

esoergel Oct 1, 2024

esoergel Oct 1, 2024

esoergel Oct 1, 2024

AddisonDunn Oct 1, 2024

esoergel Oct 1, 2024

esoergel Oct 1, 2024

esoergel Oct 1, 2024

nospame commented Oct 1, 2024

		<div id="sidebar-region" class="noprint-sub-container mt-3"></div>
		<div id="content-container" class="container mt-3">

		onDomRefresh: function () {
		this.handleSmallScreenChange(cloudcareUtils.smallScreenIsEnabled());

Make Persistent Menu Navigation a Sidebar #35142

Are you sure you want to change the base?

Make Persistent Menu Navigation a Sidebar #35142

Conversation

AddisonDunn commented Sep 18, 2024

Product Description

Technical Summary

Safety Assurance

Safety story

QA Plan

Rollback instructions

Labels & Review

nospame left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AddisonDunn commented Oct 1, 2024

esoergel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nospame commented Oct 1, 2024

nospame left a comment •

edited

Loading