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

Make Persistent Menu Navigation a Sidebar #35142

Open
wants to merge 9 commits into
base: es/2sticky2furious
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ hqDefine('cloudcare/js/debugger/debugger', [
self.registeredTabIds = self.options.tabs;
self.tabs = DebuggerTabs;

self.smallScreenIsEnabled = ko.observable(options.smallScreenIsEnabled);
$.subscribe('smallScreenIsEnabled', function (e, enabled) {
self.smallScreenIsEnabled(enabled);
});

self.evalXPath = new EvaluateXPath(options);
self.isMinimized = ko.observable(true);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ hqDefine("cloudcare/js/form_entry/form_ui", [
'cloudcare/js/utils',
'cloudcare/js/form_entry/const',
'cloudcare/js/form_entry/entries',
'cloudcare/js/formplayer/users/models',
'cloudcare/js/form_entry/utils',
'jquery-tiny-pubsub/dist/ba-tiny-pubsub', // $.pubsub
], function (
Expand All @@ -23,6 +24,7 @@ hqDefine("cloudcare/js/form_entry/form_ui", [
cloudcareUtils,
constants,
entries,
UsersModels,
formEntryUtils
) {
var groupNum = 0;
Expand Down Expand Up @@ -462,6 +464,8 @@ hqDefine("cloudcare/js/form_entry/form_ui", [
self.hasSubmitAttempted = ko.observable(false);
self.isSubmitting = ko.observable(false);
self.isAnchoredSubmitStyle = toggles.toggleEnabled('WEB_APPS_ANCHORED_SUBMIT');
self.isAppPreview = UsersModels.getCurrentUser().isAppPreview;
self.smallScreenIsEnabled = cloudcareUtils.smallScreenIsEnabled();

self.currentIndex = ko.observable("0");
self.atLastIndex = ko.observable(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ hqDefine("cloudcare/js/form_entry/utils", [
'@mapbox/mapbox-gl-geocoder/dist/mapbox-gl-geocoder.min',
'hqwebapp/js/initial_page_data',
'hqwebapp/js/toggles',
'cloudcare/js/utils',
'cloudcare/js/form_entry/const',
'cloudcare/js/form_entry/errors',
'cloudcare/js/formplayer/constants',
Expand All @@ -16,6 +17,7 @@ hqDefine("cloudcare/js/form_entry/utils", [
MapboxGeocoder,
initialPageData,
toggles,
cloudcareUtils,
formEntryConst,
errors
) {
Expand Down Expand Up @@ -72,6 +74,7 @@ hqDefine("cloudcare/js/form_entry/utils", [
username: formJSON.username,
restoreAs: formJSON.restoreAs,
domain: formJSON.domain,
smallScreenIsEnabled: cloudcareUtils.smallScreenIsEnabled(),
});
ko.cleanNode($debug[0]);
$debug.koApplyBindings(cloudCareDebugger);
Expand Down
6 changes: 6 additions & 0 deletions corehq/apps/cloudcare/static/cloudcare/js/formplayer/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,11 @@ hqDefine("cloudcare/js/formplayer/app", [
$('.last').removeClass('last');
}
);

self.smallScreenListener = CloudcareUtils.smallScreenListener(smallScreenEnabled => {
$.publish('smallScreenIsEnabled', smallScreenEnabled);
});
self.smallScreenListener.listen();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to also handle stopListening somewhere?

});

FormplayerFrontend.on('configureDebugger', function () {
Expand Down Expand Up @@ -424,6 +429,7 @@ hqDefine("cloudcare/js/formplayer/app", [
tabs: [
TabIDs.EVAL_XPATH,
],
smallScreenIsEnabled: CloudcareUtils.smallScreenIsEnabled(),
});
ko.cleanNode($debug[0]);
$debug.koApplyBindings(cloudCareDebugger);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1546,9 +1546,39 @@ hqDefine("cloudcare/js/formplayer/menus/views", [
events: {
'click #app-main': 'onClickAppMain',
},
handleSmallScreenChange: function (smallScreenEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to be able to approach this with something like this:

<div id="persistent-menu-content" style="display: none;">
  ...
</div>
<div id="persistent-menu-small">
  <button data-bs-toggle="offcanvas">...</button>
  <div class="offcanvas">
    {{ persistent-menu-content }}
  </div>
</div>
<div id="persistent-menu-big">
  <button data-bs-toggle="collapse">...</button>
  <div class="collapse show">
    {{ persistent-menu-content }}
  </div>
</div>

Where the containing context is swapped depending on which screen size is active, or perhaps the same DOM is displayed in two places, and we use media queries to figure out which to hide and which to show. I don't know a good way to do that though. Had the same problem with the login-as banner/pill.

const offcanvas = 'offcanvas';
const collapse = 'collapse';
const containerDesktopClasses = collapse + ' show position-relative';
const containerMobileClasses = offcanvas + ' offcanvas-start';
if (smallScreenEnabled) {
$('#persistent-menu-container').removeClass(containerDesktopClasses);
$('#persistent-menu-container').addClass(containerMobileClasses);
$('#persistent-menu-arrow-toggle').attr('aria-expanded', false);
$('#close-button').removeAttr('data-bs-toggle');
$('#close-button').attr('data-bs-dismiss', offcanvas);
$('#persistent-menu-arrow-toggle').attr('data-bs-toggle', offcanvas);
} else {
$('#persistent-menu-container').removeClass(containerMobileClasses);
$('#persistent-menu-container').addClass(containerDesktopClasses);
$('#persistent-menu-arrow-toggle').attr('aria-expanded', true);
$('#close-button').removeAttr('data-bs-dismiss');
$('#close-button').attr('data-bs-toggle', collapse);
$('#persistent-menu-arrow-toggle').attr('data-bs-toggle', collapse);
}
},
initialize: function (options) {
self.smallScreenListener = cloudcareUtils.smallScreenListener(smallScreenEnabled => {
this.handleSmallScreenChange(smallScreenEnabled);
});
self.smallScreenListener.listen();
},
onRender: function () {
this.showChildView('menu', new PersistentMenuListView({collection: this.collection}));
},
onDomRefresh: function () {
this.handleSmallScreenChange(cloudcareUtils.smallScreenIsEnabled());
Comment on lines +1582 to +1583
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
templateContext: function () {
const appId = formplayerUtils.currentUrlToObject().appId,
currentApp = AppsAPI.getAppEntity(appId),
Expand All @@ -1562,15 +1592,6 @@ hqDefine("cloudcare/js/formplayer/menus/views", [
onClickAppMain: function () {
FormplayerFrontend.trigger("persistentMenuSelect");
},
onBeforeDetach: function () {
// Be sure to hide offcanvas element so scroll works properly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this not an issue anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't appear to be.

const openedCanvas = bootstrap.Offcanvas.getInstance(
document.getElementById('persistent-menu-container')
);
if (openedCanvas) {
openedCanvas.hide();
}
},
});

return {
Expand Down
2 changes: 1 addition & 1 deletion corehq/apps/cloudcare/static/cloudcare/js/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ hqDefine('cloudcare/js/utils', [

var getRegionContainer = function () {
const RegionContainer = Marionette.View.extend({
el: "#menu-container",
el: "#main-container",

regions: {
main: "#menu-region",
Expand Down
32 changes: 18 additions & 14 deletions corehq/apps/cloudcare/templates/cloudcare/formplayer_home.html
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@

<div id="cloudcare-main" class="cloudcare-home-content">
<section id="cases"></section>
<div id="menu-container">
<div id="main-container" class="d-lg-flex flex-column">
<section id="formplayer-progress-container"></section>
<nav class="navbar navbar-cloudcare">
<div class="d-flex container-fluid">
Expand All @@ -102,27 +102,31 @@
<div class="d-lg-none">
<div id="mobile-restore-as-region"></div>
</div>
<div id="breadcrumb-region" class="print-container sticky-top"></div>
AddisonDunn marked this conversation as resolved.
Show resolved Hide resolved
<div id="breadcrumb-region" class="print-container"></div>
<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"></div>
<div id="content-container" class="container">
<div id="menu-region" class="print-container"></div>
<section id="webforms" data-bind="
template: {
name: 'form-fullform-ko-template',
afterRender: afterRender
}">
</section>
<div id="content-plus-persistent-menu-container" class="flex-grow-1 d-lg-flex">
<div id="persistent-menu-region"></div>
<div id="content-plus-version-info-container" class="h-100 flex-grow-1 overflow-scroll d-lg-flex-column">
<div id="sidebar-and-content" class="d-lg-flex justify-content-center m-3">
<div id="sidebar-region" class="noprint-sub-container me-3"></div>
<div id="content-container" class="mx-0 flex-grow-1">
<div id="menu-region" class="print-container"></div>
<section id="webforms" data-bind="
template: {
name: 'form-fullform-ko-template',
afterRender: afterRender
}">
</section>
</div>
</div>
<small id="version-info" class="pb-5"></small>
</div>
</div>
<div id="persistent-menu-region"></div>
</div>
<div role="region" id="sr-notification-region" class="sr-only" aria-live="assertive" aria-relevant="all"></div>
<small id="version-info"></small>
{% if request.couch_user.can_edit_data %}
<section data-bind="template: { name: 'instance-viewer-ko-template' }"
id="cloudcare-debugger"></section>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
'debugger-minimized': isMinimized,
'debugger-maximized': !isMinimized(),
'debugger-updating': updating
}
},
visible: !smallScreenIsEnabled(),
Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

">
<!-- Tab title -->
<div class="debugger-tab-title clickable" tabindex="0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 #}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I did that and I also didn't prefer it. BTW this is only applicable for the sticky submit button (Where the submit button is shown as a bar anchored to the bottom of the screen) But from what I recall, it was tricky and involved some overhauling since the submit button and data preview bar and not within the same container. Also, both the anchored submit button and data preview bar are positioned using position:fixed and I couldn't figure out a way to make fixed items "float" even if they're on the same z-index.

}"
>
<div class="col-12 text-center submit">
Expand Down
Original file line number Diff line number Diff line change
@@ -1,35 +1,32 @@
{% load i18n %}

<script type="text/template" id="persistent-menu-template">
<a type="button"
<a id="persistent-menu-arrow-toggle"
type="button"
Comment on lines +4 to +5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could this just be a button element instead?

href="#"
tabindex="1"
data-bs-toggle="offcanvas"
data-bs-target="#persistent-menu-container"
aria-expanded="false"
aria-expanded="true"
aria-controls="persistent-menu-container"
class="position-fixed top-50 start-0 rounded-end persistent-menu-toggle">
<i class="fa fa-chevron-right"></i>
</a>
<div id="persistent-menu-container" class="offcanvas offcanvas-start w-auto" tabindex="-1" aria-labelledby="persistent-menu-label">
<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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

data-bs-target="#persistent-menu-container" aria-label="Close"></button>
<h2 class="title position-relative me-4" id="persistent-menu-label">
<% if (imageUri) { %>
<i class="fcc persistent-menu-image align-self-start flex-shrink-0" aria-hidden="true"
style="background-image: url('<%- imageUri %>');"></i>
style="background-image: url('<%- imageUri %>');"></i>
<% } else { %>
<i class="fcc fcc-flower persistent-menu-image align-self-start flex-shrink-0" aria-hidden="true"></i>
<i class="fcc fcc-flower persistent-menu-image" aria-hidden="true"></i>
<% } %>
<h5 class="offcanvas-title position-relative mx-2 flex-grow-1" id="persistent-menu-label">
<a id="app-main" class="stretched-link text-reset d-block" href="#"
data-bs-toggle="offcanvas" data-bs-target="#persistent-menu-container">
<%- appName %>
</a>
</h5>
<button type="button" class="btn-close text-reset p-3" data-bs-dismiss="offcanvas" aria-label="Close"></button>
</div>
<div class="offcanvas-body persistent-menu-max-width">
<div id="persistent-menu-content" class="persistent-menu"></div>
</div>
<a id="app-main" class="align-middle stretched-link text-reset" href="#">
<%- appName %>
</a>
</h2>
<div id="persistent-menu-content" class="persistent-menu"></div>
</div>
</script>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
{% endif %}

<section id="cases"></section>
<div id="menu-container">
<div id="main-container">
<div id="formplayer-progress-container"></div>
<section id="cloudcare-notifications" class="notifications-container"></section>
<div id="restore-as-region"></div>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.hq-container {
padding-bottom: 0;
margin-bottom: 0;
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
@import "mixins"; // media-breakpoint-up and media-breakpoint-down

@import "formplayer-common-main";
@import "corehq_overrides";
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ formplayer, please make changes to the cloudcare styles.
**/

$breadcrumb-height-cloudcare: 36px;
$cloudcare-nav-height: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$minimized-debugger-height: 30px;

@import "formplayer-webapp/content";
@import "formplayer-webapp/navbar";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@
background: transparent !important;
> div {
background: white;
margin-bottom: 20px;
display: grid;
container-type: inline-size;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
body {
overflow-x: hidden;
overflow: hidden;
background-color: $cc-bg;
}

.cloudcare-home-content {
padding: 0 0 2rem;
#cloudcare-main, #main-container {
height: 100vh;
}


#hq-footer {
display: none;
}
Expand Down Expand Up @@ -35,8 +34,17 @@ body {
padding: 14px 8px 7px;
}

#content-plus-persistent-menu-container {
height: calc(100vh - #{$cloudcare-nav-height});
}

.has-breadcrumbs #content-plus-persistent-menu-container {
height: calc(100vh - #{$cloudcare-nav-height} - $breadcrumb-height-cloudcare);
}

#content-container {
background: transparent;
max-width: 1000px;
}

@media print {
Expand All @@ -52,10 +60,6 @@ body {
display: none !important;
}

#cloudcare-main {
padding: 0px;
}

video {
border: 1px solid #ddd;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ $form-text-size: 16px; // If updating, update .checkbox, .radio margin-top to fi
.form-container {
background-color: white;
box-shadow: 0 0 10px 2px rgba(0,0,0,.1);
margin-bottom: 2rem;
font-size: $form-text-size; // Don't overshadow inputs

.page-header h1 {
Expand Down
Loading