Feature: Make it possible to configure if admin bar loads on page load #1449

houmark · 2018-06-26T09:09:12Z

This PR introduces two new configuration options in app.js:

modules: {
  'apostrophe-admin-bar': {
    openOnLoad: false,          // default true
    openOnHomepageLoad: true,           // default true
    closeDelay: 5000           // default 3000 (ms)
  },
    ......
};

Features:

It's possible to set openOnLoad to false, making the admin bar stay closed on page loads. If not set defaults to true, opening the admin bar on every page load.
It's possible to configure the timeout until the admin bar closes after load if openOnLoad is true or undefined / not set. closeDelay configures the time in milliseconds before the admin bar closes. If not set or set to 0, it will default back to 3000.
Makes it possible to open on the homepage using openOnHomepage, but still keep it closed on subpages of the site.

Thoughts:

I thought about making it possible to set closeDelay to 0 and make it stay open infinite or until the user clicks the Apostrophe logo, but decided to not complicate the code further unless there's a want from core devs for this. In that case, let me know and I will update this PR.

This needs a bit of documentation, but I am not sure how that works, I hope the above text helps adding it for whoever has access/knowledge.

Add new configuration to control if admin bar opens on page load.

houmark · 2018-06-26T14:55:38Z

After a bit of sleep I'm considering to rename stayOpenTimeout to closeDelay or something like that. I am open to suggestions, please let me know what's your preference and I will update this PR.

stuartromanek · 2018-06-26T15:16:46Z

Hey @houmark this is awesome work.

The Apostrophe team had enhancement to this feature in mind and it is slightly more than what you have here, maybe you'd be willing to work it into this branch?

in addition to a binary open/close state, we had planned to implement a 'only open on the homepage' mode that would be open by default and close with a timeout.

I think this would fit on top of your implementation, double checking another flag and comparing it to window.location.pathname === '/' and using the fallback stayOpenTimeout value of 3000 for the timeout.

What do you think about adding this?

houmark · 2018-06-26T15:23:44Z

Thanks for your feedback @stuartromanek.

I would, but I am not sure checking window.location.pathname === '/' is enough as prefix may come into play here? Also, people could "wrongfully" link to the home page with a path of the home page? Maybe there's a better way to check if this page is the outer most page?

Do you have an opinion on the infinite open which would also overrule this new home page setting?

stuartromanek · 2018-06-26T15:41:58Z

Didn't think pathname was sufficient either.. will dig around a bit more to see if there is a better way to know you're on the homepage from the front-end.

As for infinite open, it is not a core want right now but since we're going down this road I'd like to have it. It would make a lot of sense to use if a dev unfloats the admin bar and puts it back in flow as an element.

abea · 2018-06-26T15:43:13Z

Would applying some data attribute if the page had no ancestors work?

houmark · 2018-06-26T15:53:09Z

@abea Yes. There's no way to pass on the current page "meta data" from the admin-bar module as it doesn't run on page load, but only on app start, so it's JavaScript only, which means some other part of Apostrophe would need to add some kind of boolean for the user.js JavaScript to look for inside the DOM.

I do have a window.location.pathname === '/' working now, but that's probably only because my test site does not have any prefix. I will commit that in, pending help from someone in your end to add the more safe data attribute.

Add `openOnHomepage` option which defaults to true and will keep the admin bar open on the front page using the `closeDelay` (default 3000ms) value (needs improvement, see comments in PR). Renamed `stayOpenTimeout` to `closeDelay` as I think it's a better name.

abea · 2018-06-26T16:15:18Z

I'm thinking that in apostrophe-pages there'd be a beforeShow function checking on the page's ancestors, then passing a property through to base.html or whatever high level template.

houmark · 2018-06-26T16:19:12Z

@abea I'll take a look. @stuartromanek would it be accepted to put a data attribute on the body tag of all pages or at least the home page for this feature to make it in? I don't see any other safe element to add it to, the only other option being html

This will keep the admin bar option infinitely.

stuartromanek · 2018-06-26T16:56:05Z

adding data-apos-home to the body of lib/modules/apostrophe-templates/views/outerLayoutBase.html should be sufficient.

houmark · 2018-06-26T17:01:46Z

I was considering to use the internal existing req.data.aposBodyClasses to set a class on the body tag of the home page only. Currently trying to find the best place to determine what is the home page to set that value in apostrophe-pages. It's some hardcore stuff going on there, so I'm not making much progress. I could use a 5 min session with @boutell to point me to the right place to add this.

Adding the data-apos-home is another option which would require a template update, which may not be a problem, but could be for people that have completely overwritten the outerLayoutBase.html, but I guess adding a new block to the outerLayoutBase.html could work?

houmark · 2018-06-26T17:43:25Z

Please see latest commit.

I decided to use the body class for now. I think the place I am hooking into the core is pretty much okay, but I am curious to hear feedback from @boutell on that. Open for changing this concept, but not sure how to add the block and write to it from where I am detecting level 0 in the api of apostrophe-pages and also concerned about backwards compatibility with that approach. Also Apostrophe core itself has yet to use req.data.aposBodyClass and I think this is a very good use case. The added side effect is that it's more simple for devs to use this class in the code (I know that would also be possible with the attribute, but a class is more "known").

Awaiting feedback, but considering this feature complete for now.

stuartromanek · 2018-06-26T18:34:00Z

hey @houmark , our house practice is to using data attributes on DOM elements being monitored by JS, it's a reminder to a developer looking at a template that more is happening here beyond CSS styling.

@boutell thoughts on placement? a similar hook for programmatically adding data attrs?

houmark · 2018-06-26T19:04:52Z

Updated code to use "house practice".

(would be great with some core guidelines and best practices documentation page when contributing — but I'm sure that's on the todo list)

stuartromanek · 2018-06-27T14:42:04Z

openOnLoad: false being overridden by the baked-in default openOnHomepage: true doesn't feel quite right because I am explicitly shutting it off.

I feel like the flow should be

Default (no dev setting): openOnHomepage behavior
openOnLoad: false: admin bar never opens
openOnLoad: true: admin bar always opens

Not sure if the case of openOnHomepage: false + openOnLoad: true needs to be addressed, but maybe yes for completeness?

@Runyx @grdunn @abea can you weigh in defaults and this API?

stuartromanek · 2018-06-27T14:42:55Z

lib/modules/apostrophe-admin-bar/public/js/user.js

@@ -19,8 +19,14 @@ apos.define('apostrophe-admin-bar', {

    self.enhance = function() {
      var $bar = $('[data-apos-admin-bar]');
+      var isHomepage = $('[data-apos-home]');


prefix this var with $ to denote jQuery object

boutell · 2018-06-27T14:51:38Z

I'd also write `var isHomepage = $('[data-apos-home']).length`, so it can actually be treated as the boolean flag its name implies.

…

On Wed, Jun 27, 2018 at 10:45 AM, Stuart Romanek ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In lib/modules/apostrophe-admin-bar/public/js/user.js <#1449 (comment)> : > @@ -19,8 +19,14 @@ apos.define('apostrophe-admin-bar', { self.enhance = function() { var $bar = $('[data-apos-admin-bar]'); + var isHomepage = $('[data-apos-home]'); prefix this var with $ to denote jQuery object — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1449 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB9fSuUxeF_hSMM8sg_qMXCvTlHq215ks5uA5qngaJpZM4U3iZU> .

-- *THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT* P'UNK AVENUE | (215) 755-1330 | punkave.com

abea · 2018-06-27T15:00:45Z

Regarding defaults, are we not worried about a behavior BC break? If not, I think what @stuartromanek wrote looks good. I'm not sure I see a problem with openOnHome:true overriding openOnLoad: false for that page.

If so, we could do:

Default (no dev setting): openOnLoad: true behavior
openOnLoad: false: admin bar never opens
openOnLoad: false, openOnHome: true : admin bar opens on home but nowhere else

Not as neat, but would resolve a break. It also wouldn't establish opening only on the homepage as the default.

In the case openOnHomepage: false, openOnLoad: true, openOnLoad should win.

houmark · 2018-06-27T16:20:18Z

If we're going to support pushing data attributes on the body element, and I see Stuart's argument there, then we need to introduce a new method apos.templates.addBodyDataAttribute which would take both an attribute name and a value. The data- prefix would be added automatically.

@boutell Thanks for the hint on the templates API function. I agree on introducing a more generic core API to add this as it can be used by anything else also. I can add this API, but would you accept it as a part of this PR, or should it be its own PR? This PR would rely on the other one being added, so it would complicate testing more.

houmark · 2018-06-27T16:33:08Z

@abea Your latest comment on default behavior is, unless I missed a detail, how it currently works and I did it like that to ensure backwards compatibility.

To sum up: If you don't add anything to app.js, then it will continue to work exactly as before. But the new option openOnHomepage is default to true and that overwrites the behavior for the homepage only, which means it will still open with the default 3000ms timeout, if you don't add any setting.

So to completely disable auto opening, you add both onOnLoad: false; and openOnHomepage: false;. This was how I understood the added homepage feature from @stuartromanek but I may have gotten his idea wrong.

In any case, I can make openOnLoad become the superseding configuration here which means you would only need to set openOnLoad: false; to fully disable the auto load on any page including the homepage.

Again, if you don't change anything when upgrading Apostrophe, things will continue to work as now; admin bar opens on all pages with a delay of 3000ms before they close.

I personally like it as it is right now, because it gives you the most flexibility in configuration. You can keep it closed on all subpages, but still open it on the front page, which helps new editors to understand "what this icon is all about". I even considered mixing this with user/group based settings for the logged in user, but decided to keep v1 simple (this would allow for the admin group keeping it always closed, but a normal user group always opened etc.).

boutell · 2018-06-27T17:03:49Z

I would make it `openOnHomepageLoad` for consistency. Like the general approach fine. @houmark it would be fine to include the addition of that API in the PR. Look at the addBodyClass one to see the pattern.

…

On Wed, Jun 27, 2018 at 12:33 PM, houmark ***@***.***> wrote: @abea <https://github.com/abea> Your latest comment on default behavior is, unless I missed a detail, how it currently works and I did it like that to ensure backwards compatibility. *To sum up:* If you don't add anything to app.js, then it will continue to work exactly as before. But the new option openOnHomepage is default to true and that overwrites the behavior for the homepage only, which means it will still open with the default 3000ms timeout, if you don't add any setting. So to completely disable auto opening, you add both onOnLoad: false; and openOnHomepage: false;. This was how I understood the added homepage feature from @stuartromanek <https://github.com/stuartromanek> but I may have gotten his idea wrong. In any case, I can make openOnLoad become the superseding configuration here which means you would only need to set openOnLoad: false; to fully disable the auto load on any page including the homepage. Again, if you don't change anything when upgrading Apostrophe, things will continue to work as now; admin bar opens on all pages with a delay of 3000ms before they close. I personally like it as it is right now, because it gives you the most flexibility in configuration. You can keep it closed on all subpages, but still open it on the front page, which helps new editors to understand "what this icon is all about". I even considered mixing this with user/group based settings for the logged in user, but decided to keep v1 simple (this would allow for the admin group keeping it always closed, but a normal user group always opened etc.). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1449 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB9fdll6nBuX9N2eoHcvZPrexJHGac1ks5uA7PFgaJpZM4U3iZU> .

-- *THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT* P'UNK AVENUE | (215) 755-1330 | punkave.com

houmark · 2018-06-27T17:12:02Z

Thanks @boutell, got it. This should be ready very soon.

One more question. For people to be able to rely on the apos-homepage I was considering to add a default apos-subpage which will be on all subpages also, now that I'm at it? Would just be the else on the level === 0. Should I?

boutell · 2018-06-27T17:20:17Z

How about `data-apos-level` and it's the value of the `level` property? That has many uses.

…

On Wed, Jun 27, 2018 at 1:12 PM, houmark ***@***.***> wrote: Thanks @boutell <https://github.com/boutell>, got it. This should be ready very soon. One more question. For people to be able to rely on the apos-homepage I was considering to add a default apos-subpage which will be on all subpages also, now that I'm at it? Would just be the else on the level === 0. Should I? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1449 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB9feVod5_5S-xDTXi_b4o5_3lETfPlks5uA7z8gaJpZM4U3iZU> .

-- *THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT* P'UNK AVENUE | (215) 755-1330 | punkave.com

houmark · 2018-06-27T18:31:31Z

It could, but since this is attribute and value only, I don't see the need for an object as there's no apparent use case for adding more key/values to the object?

houmark · 2018-06-27T18:33:34Z

Only if you also make it possible to wrap the object in an array, so that you can pass multiple attributes at once. But this makes it a bit more complex and this API function can be called multiple times if you need multiple attributes added (with my latest edition, that I am about to push).

And sorry, I missed your "array of objects" here in my first read which led to my weird comments.

This means it can only receive one attribute per call but it can be called multiple times of course.

houmark · 2018-06-27T18:35:16Z

lib/modules/apostrophe-templates/index.js

      req.data.aposBodyDataAttributes = (req.data.aposBodyDataAttributes ? (req.data.aposBodyDataAttributes + ' ') : ' ') +
-        ('data-' + _.trim(bodyAttribute)).split(' ').join(' data-');
+        'data-' + (!_.isUndefined(value) ? (name + '=' + value + '') : name);


I need to also check for name not being undefined / empty. Will push another commit for that, which will just return.

Question goes. I wanted the value to be wrapped in " (double quotes) but it's being escaped in the body tag. How do I "bless" this internally to prevent that @boutell?

Also trim to avoid rendering a broken attribute

houmark · 2018-06-27T18:50:41Z

I've done the changes requested, added some inline comments here, awaiting feedback.

…rameter Passing an object in our first use case.

houmark · 2018-06-27T19:50:25Z

I went all in on this one and made it possible to pass an object, an array or a string (string for backwards compatibility haha, not). I hope you like that approach.

boutell · 2018-06-29T20:30:53Z

lib/modules/apostrophe-templates/index.js

+    // is attached to `req.data`, where `req.data.aposBodyDataAttributes` is built up
+    // using `name` as the attribute name which is automatically prepended with "data-"
+    // and the optional `value` argument
+    // Also receives a single object or an array of objects with name and value key pairs


I think a better use for an object syntax would be an object like:

{ foo: 'value', bar: 'other value' }

That feels natural for adding attributes. In fact I think the array syntax could be omitted in favor of this.

boutell · 2018-06-29T20:32:31Z

lib/modules/apostrophe-templates/index.js

+          return;
+        }
+        req.data.aposBodyDataAttributes = (req.data.aposBodyDataAttributes ? (req.data.aposBodyDataAttributes + ' ') : ' ') +
+        'data-' + (!_.isUndefined(attribute.value) && attribute.value.toString().length > 0 ? (attribute.name + ('="' + attribute.value + '"')) : attribute.name);


Since we're using "safe" here, we need to make sure these values are actually safe. The attribute name and value should be passed through apos.utils.escapeHtml.

houmark · 2018-06-29T22:06:31Z

Okay, so remove the array option, and key/value objects which also can be multiple and thus you can add multiple attributes in one call to the API function? Should I keep the string option to make it possible to just call the function with 3 arguments; req, name, value or do you want this to be object only for a simpler code base?

I will also do apos.utils.escapeHtml on the attribute name and value.

houmark · 2018-06-29T22:07:08Z

@boutell forgot to mention you, please let me know so I can try to get this one right the next time :)

Will still accept a string value for attribute name and value. Array no longer supported, but the object can be multiple key / value pairs.

houmark · 2018-06-29T23:17:25Z

I have now updated the code to accept an object of key / value based pairs. It will still accept 2 arguments (after req) as strings to just pass one data attribute.

I'm once more back to be pending on reviews.

boutell

Thank you for this PR - we are close!

boutell · 2018-07-02T14:10:15Z

lib/modules/apostrophe-admin-bar/public/js/user.js

@@ -19,8 +19,14 @@ apos.define('apostrophe-admin-bar', {

    self.enhance = function() {
      var $bar = $('[data-apos-admin-bar]');
+      var $isHomepage = $('[data-apos-level="0"]').length;


Now that this is a simple boolean and not a jquery object anymore... which is great... the $ is no longer appropriate. Sorry for the hassle. I have a lot of PRs to look at today or I'd slip this change in myself!

boutell · 2018-07-02T14:12:33Z

lib/modules/apostrophe-templates/index.js

+    // is attached to `req.data`, where `req.data.aposBodyDataAttributes` is built up
+    // using `name` as the attribute name which is automatically prepended with "data-"
+    // and the optional `value` argument
+    // Also receives a single object or an array of objects with name and value key pairs


No longer quite accurate, we removed the array syntax but you're still talking about it in the doc comment.

boutell · 2018-07-02T14:13:33Z

lib/modules/apostrophe-templates/index.js

+
+    self.addBodyDataAttribute = function(req, name, value = '') {
+      var values = {};
+      if (_.isObject(name) && !_.isArray(name) && !_.isFunction(name)) {


You can drop all checks here other than _.isObject(name) as the only allowed syntaxes now are the "name, value" syntax and the object syntax. Ditto the other places _.isArray() is still around, shouldn't need to do that.

houmark · 2018-07-02T19:16:02Z

I think with my latest changes we should be good. I also merged master back into this branch and there should be no issues.

stuartromanek · 2018-07-05T14:05:07Z

@boutell are you good on this implementation?

@houmark can you update the branch with master?

houmark · 2018-07-05T19:28:25Z

Branch is now up to date with master. Ready to merge :)

boutell · 2018-07-06T13:12:23Z

Thanks!

Feature: Make it possible to configure if admin bar loads on page load

c498bd5

Add new configuration to control if admin bar opens on page load.

houmark mentioned this pull request Jun 26, 2018

Change admin bar default state to closed? #1419

Closed

Add possibility to set closeDelay to 0

4d7fd63

This will keep the admin bar option infinitely.

Improve homepage detection

6e8fdca

Use data attribute instead of body class

c182b87

stuartromanek requested changes Jun 27, 2018

View reviewed changes

houmark added 2 commits June 27, 2018 12:07

Add $ to indicate we are in jQuery land

16f2f79

Rename openOnHomepage to openOnHomepageLoad for consistency

1dc5173

Make addBodyDataAttribute use 3 arguments, req, name, value

5412a56

This means it can only receive one attribute per call but it can be called multiple times of course.

houmark commented Jun 27, 2018

View reviewed changes

Some safe checking for empty values

7bfc0e0

Also trim to avoid rendering a broken attribute

houmark added 2 commits June 27, 2018 14:13

Add documentation for new configuration options

8233612

Make it possible to pass an object, an array or a string as second pa…

962f5b1

…rameter Passing an object in our first use case.

houmark added 2 commits June 27, 2018 14:56

Update inline documentation

2125f5e

Add double quotes on the data attribute

2e7508f

boutell requested changes Jun 29, 2018

View reviewed changes

Merge branch 'master' into feature-admin-bar-open-on-load

e88ae40

Change addBodyDataAttribute to use key / value based object

ba9259a

Will still accept a string value for attribute name and value. Array no longer supported, but the object can be multiple key / value pairs.

boutell requested changes Jul 2, 2018

View reviewed changes

houmark added 3 commits July 2, 2018 14:01

Merge branch 'master' into feature-admin-bar-open-on-load

8bb60b1

Remove $ on variable as it's no longer jQuery object

bca961c

Update documentation. Remove array check.

56d69a1

Merge branch 'master' into feature-admin-bar-open-on-load

d9a74fa

stuartromanek approved these changes Jul 5, 2018

View reviewed changes

Merge branch 'master' into feature-admin-bar-open-on-load

d8dfa60

boutell approved these changes Jul 6, 2018

View reviewed changes

boutell merged commit 0a8c59d into apostrophecms:master Jul 6, 2018

Feature: Make it possible to configure if admin bar loads on page load #1449

Feature: Make it possible to configure if admin bar loads on page load #1449

Conversation

houmark commented Jun 26, 2018 • edited

houmark commented Jun 26, 2018

stuartromanek commented Jun 26, 2018

houmark commented Jun 26, 2018

stuartromanek commented Jun 26, 2018

abea commented Jun 26, 2018

houmark commented Jun 26, 2018

abea commented Jun 26, 2018

houmark commented Jun 26, 2018

stuartromanek commented Jun 26, 2018

houmark commented Jun 26, 2018

houmark commented Jun 26, 2018

stuartromanek commented Jun 26, 2018

houmark commented Jun 26, 2018

stuartromanek commented Jun 27, 2018

Choose a reason for hiding this comment

boutell commented Jun 27, 2018 via email

abea commented Jun 27, 2018

houmark commented Jun 27, 2018

houmark commented Jun 27, 2018

boutell commented Jun 27, 2018 via email

houmark commented Jun 27, 2018

boutell commented Jun 27, 2018 via email

houmark commented Jun 27, 2018

houmark commented Jun 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houmark commented Jun 27, 2018

houmark commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houmark commented Jun 29, 2018

houmark commented Jun 29, 2018

houmark commented Jun 29, 2018

boutell 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

houmark commented Jul 2, 2018

stuartromanek commented Jul 5, 2018

houmark commented Jul 5, 2018

boutell commented Jul 6, 2018

houmark commented Jun 26, 2018 •

edited

houmark commented Jun 27, 2018 •

edited