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

Persistent Collapsible Containers #3638

Merged
merged 59 commits into from Jul 18, 2019

Conversation

@flourish86
Copy link
Contributor

commented Nov 29, 2018

No description provided.

@flourish86 flourish86 requested a review from lippserd Nov 29, 2018

@flourish86 flourish86 force-pushed the feature/js-collapsible-containers branch from 05643f3 to 34307d3 Jan 7, 2019

@flourish86

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

#34307d3 fixes collapsible container id handling.

#f07e3c0 adds demo output for collapsible containers in detail view.

@nilmerg nilmerg assigned nilmerg and unassigned lippserd Jun 6, 2019

@nilmerg nilmerg force-pushed the feature/js-collapsible-containers branch from f07e3c0 to 0d8352a Jun 6, 2019

@nilmerg

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

So... I've reworked this now quite a bit. The previous implementation has been plagued by inconsistencies regarding the used jQuery functions and made too many assumptions how to make containers collapsible. Having an extra data attribute just for id's didn't seem necessary to me as well.

The general usage of this behavior is now as follows:

<div class="collapsible">
    <!-- This will collapse the entire content to the default height of 36px. The height can be customized with `data-visible-height` -->
</div>

<table class="collapsible">
    <!-- This will collapse `tbody` to the default amount (2) of `tr`s. The amount can be customized with `data-visible-rows` -->
</table>

<ul class="collapsible"> <!-- may also be an ol -->
    <!-- This will collapse to the default amount (2) of `li`s. The amount can be customized with `data-visible-rows` -->
</ul>

Note that none of these have an id. collapsible.js utilizes internally ìcinga.utils.getCSSPath now and automatically identifies an appropriate id in the container's vicinity.

On shutdown (window unload, controlled by icinga.js which now destroys behaviors just like modules) the recently expanded collapsibles are preserved in the browsers local storage.

Possible issues:

  • The behavior assumes now that every id is unique. This will cause conflicts with elements having the same id but different content. (e.g. #plugin-output) Though, this may very well be of advantage as well since someone expanding plugin output may want to do this everywhere else as well.
  • Protected ids are useless at the moment. A window's id is not stored in local storage thus will definitely change and the container's id will get invalid.
  • There's no cleanup of the local storage currently. Containers expanded at shutdown are written into storage and not removed until either sometime nothing is expanded at shutdown (storage is cleared) or they'll get manually collapsed.
@lippserd
Copy link
Member

left a comment

In addition to the dummy markup, the behavior JS and CSS have some style issues, e.g. spaces after the function keyword for JS and missing spaces after commas for CSS. Further, the behavior does not work for me in Chrome at all. Please see the attached screenshot. The controls seem a little odd and there is no functionality when I click the expand button. There are no JS errors though.
I only had a quick look at the code, but does the state saving and loading work with two or more open tabs?

Screenshot 2019-06-24 at 11 11 00

@nilmerg

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

I've rebased the branch, are you sure you've got a working copy? For me everything works as expected, Chromium though. The state saving.. no that can't really cope with multiple simultaneously open tabs. I just had multiple sessions in mind. The local storage only allows simple key-value pairs so the behavior utilizes a single value to store its state. Though, adjusting the storage transparently/instantly, while making it less efficient, would also save us the need to destroy the behavior as we wouldn't need the unload event.

@nilmerg nilmerg force-pushed the feature/js-collapsible-containers branch from ae0e7ec to 184706c Jun 25, 2019

@nilmerg

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Fixed js style issues. Didn't found the css style issues you've mentioned.

Made it work with multiple tabs by directly speaking to the localstorage when updating the state. I'm well aware of the fact that this is highly inefficient, but unless we make use of a dedicated web-worker or such this will not be an easy thing to optimize. If you think this is reasonable, I'll happily do so. Though, since that's another thing that might require a more sophisticated implementation, we should probably wait for it until v2.8.

@lippserd

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

I still have the style and functionality issues. Safari has no style issues but it does not expand the container.

Screenshot 2019-06-27 at 15 01 01

@lippserd

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@flourish86 Could you please test this?

@flourish86

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

I can't reproduve any of the mentioned issues.

@lippserd: The reason why it might not collapse in your case is, because there aren't sufficient rows.

I also discovered another issue: The collapsible state isn't calculated correctly when you switch from two column to one column layout.

See here (from Safari):

Screen Recording 2019-06-27 at 17 38 24 2019-06-27 17_39_32

@nilmerg

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@flourish86 I'd rather say that for only text content a specific height (i.e. 36px) does not work since fonts tend to vary across devices 😉 I guess counting the lines is the better option here 🤔

@flourish86

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

@flourish86 I'd rather say that for only text content a specific height (i.e. 36px) does not work since fonts tend to vary across devices 😉 I guess counting the lines is the better option here 🤔

Sounds realtistic. But shouldn't one line be exactly font-size * line-height?

@nilmerg

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Sounds realtistic. But shouldn't one line be exactly font-size * line-height?

Should be, yes. Though, internet said this is not reliable as not all browsers give a unit-less value. Or even normal may be a case. And calculating all cases correctly on-demand I figured this is a nightmare since this may differ based on where text is displayed. (i.e. A dummy text content somewhere hidden can't be used as basis.)

I've committed de27445 as a work-around.

@nilmerg nilmerg requested a review from lippserd Jul 11, 2019

@lippserd
Copy link
Member

left a comment

Very great set of changes! Thanks for that 👍
I only requested some cosmetic changes. And I have a question:
Is it on purpose to leave the key in the storage if all keys got removed because of their TTL?
Because, if I collapse an item and it's the last key, the storage key is removed as well.

public/js/icinga/behavior/collapsible.js Show resolved Hide resolved
public/js/icinga/behavior/collapsible.js Outdated Show resolved Hide resolved
public/js/icinga/behavior/collapsible.js Outdated Show resolved Hide resolved
public/js/icinga/storage.js Outdated Show resolved Hide resolved
public/js/icinga/behavior/collapsible.js Outdated Show resolved Hide resolved
public/js/icinga/behavior/collapsible.js Outdated Show resolved Hide resolved
public/js/icinga/storage.js Outdated Show resolved Hide resolved

@nilmerg nilmerg force-pushed the feature/js-collapsible-containers branch from 09211b5 to b9887bf Jul 17, 2019

nilmerg added some commits Jul 8, 2019

storage.js: Write `null` instead of `undefined` to the storage
`undefined` causes the key to be ignored by JSON.stringify
storage.js: Utilize a single event listener for all storage events
It doesn't make sense to register an event listener for every
created storage instance. They're all using entirely different
keys after all.
collapsible.js: Rename event callbacks
`onExternalCollapse` => `onCollapse`
`onExternalExpansion` => `onExpand`

@nilmerg nilmerg force-pushed the feature/js-collapsible-containers branch from b9887bf to cfa3af5 Jul 17, 2019

@nilmerg nilmerg requested a review from lippserd Jul 17, 2019

@nilmerg nilmerg added this to the 2.7.0 milestone Jul 18, 2019

@nilmerg nilmerg changed the title Feature/js-collapsible-containers Persistent Collapsible Containers Jul 18, 2019

@nilmerg nilmerg merged commit 6cd9408 into master Jul 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nilmerg nilmerg deleted the feature/js-collapsible-containers branch Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.