-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
base: es/2sticky2furious
Are you sure you want to change the base?
Changes from 6 commits
a0ae928
55be2ef
842ad5c
7e2b3c1
fa979a8
4319ec4
a78a365
919ed4f
e775d4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1546,9 +1546,39 @@ hqDefine("cloudcare/js/formplayer/menus/views", [ | |
events: { | ||
'click #app-main': 'onClickAppMain', | ||
}, | ||
handleSmallScreenChange: function (smallScreenEnabled) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. out of curiosity, why is this in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}, | ||
templateContext: function () { | ||
const appId = formplayerUtils.currentUrlToObject().appId, | ||
currentApp = AppsAPI.getAppEntity(appId), | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this not an issue anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,8 @@ | |
'debugger-minimized': isMinimized, | ||
'debugger-maximized': !isMinimized(), | ||
'debugger-updating': updating | ||
} | ||
}, | ||
visible: !smallScreenIsEnabled(), | ||
Comment on lines
+10
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this bit be done with Bootstrap using classes on the <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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is doable with bootstrap classes rather than needing JS: |
||
"> | ||
<!-- Tab title --> | ||
<div class="debugger-tab-title clickable" tabindex="0" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 #} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Then I can offset other elements with just CSS without needing to replicate the set of circumstances that indicate whether that element is present: 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 Another perk of that approach is that this line could be replaced with SCSS, and you could use a reference to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}" | ||
> | ||
<div class="col-12 text-center submit"> | ||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: could this just be a |
||
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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> | ||
|
||
|
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 |
---|---|---|
|
@@ -11,6 +11,8 @@ formplayer, please make changes to the cloudcare styles. | |
**/ | ||
|
||
$breadcrumb-height-cloudcare: 36px; | ||
$cloudcare-nav-height: 30px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
$minimized-debugger-height: 30px; | ||
|
||
@import "formplayer-webapp/content"; | ||
@import "formplayer-webapp/navbar"; | ||
|
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.
Do we need to also handle
stopListening
somewhere?