jneilliii / OctoPrint-Tasmota

62 stars 16 forks source link

Switch to using popover instead of tooltip #184

Closed ManuelMcLure closed 2 years ago

ManuelMcLure commented 2 years ago

Switches from using a tooltip to using a nicely formatted/styled popover.

jneilliii commented 2 years ago

This z-index issue could be theme related but definitely think the empty space shouldn't exist if there's no content for the popover besides label.

image

ManuelMcLure commented 2 years ago

Good point. I'm not sure what would happen if I remove the <table> tag completely. I'll give it a test.

ManuelMcLure commented 2 years ago

Hmmm. An empty string for the tooltip portion still shows extra stuff. I'm looking at some options to fix that.

ManuelMcLure commented 2 years ago

I have something that works better but isn't quite right yet.

ManuelMcLure commented 2 years ago

OK - I think my latest fixes should solve the issue you were seeing.

jneilliii commented 2 years ago

Something is a little strange still. On initial page load it's still showing like before, but if I toggle the tasmota it corrects.

ManuelMcLure commented 2 years ago

It's a timing thing. I can't find a way to add the .hide_popover_content class to the popover in HTML so it's set on initial page load. I add it in onAllBound() which works mostly for me but may have a race condition. It should auto correct as soon as the poll interval occurs. Let me try adding the class to #navbar_plugin_tasmota and reading it from there and see if that works.

ManuelMcLure commented 2 years ago

That seems to work here in my tests. I've pushed a new commit. If that works I'll squash and force-push to clean up the PR.

ManuelMcLure commented 2 years ago

Actually, hold off - I'm missing something.

ManuelMcLure commented 2 years ago

No - it's working with my new commit. Let me know if you see any other issues.

jneilliii commented 2 years ago

Ah, could have probably got around that using additional bindings or classes to the get_template_configs callback like this.

{"type": "navbar", "custom_bindings": True, data_bind="some_ko_observable_to_check_for_empty_data"}

or

{"type": "navbar", "custom_bindings": True, "classes": ["hide_popover_content"]}

I was able to test multiple devices with no data, but not one with and without data configured. Would that have an effect you think since the class is assigned to the overall navbar container?

ManuelMcLure commented 2 years ago

I did some manual testing by editing tasmota.js and changing the data.sensor_data || data.energy_data condition to false, but the only devices I have are the plugs with power monitoring.

ManuelMcLure commented 2 years ago

Ah, could have probably got around that using additional bindings or classes to the get_template_configs callback like this.

Let me see if that works. That's a cleaner solution.

ManuelMcLure commented 2 years ago

Yeah - that worked.

ManuelMcLure commented 2 years ago

Oh, and on the Z-order thing, it wasn't actually Z order - it's just that the border between the title and the content of the popover was exactly at the same pixel height as the line across the "Connection" sidebar so it looked like it was bleeding through. If you collapsed the "Connection" section it was more obvious that the line was part of the popover. In any case I now turn off the border if there's no data.

jneilliii commented 2 years ago

you're totally correct about the z order thing. I forgot I had a BME280 connected to my nodemcu so configured it as a second device and as I suspected, if one device has data the device without data then expands out with no content.

jneilliii commented 2 years ago

wonder if the popover ko binding has additional options for optional class names that could be tied to the existence of data or not?

jneilliii commented 2 years ago

could probably add the css ko binding to the anchor element, just don't know if the popover div is inside the triggering element or not to assign the right class.

jneilliii commented 2 years ago

looks like you might have to do a wrapper around the anchor tag for it to work as the popover is inserted at the same level. IIRC that has it's own can of worms when it comes to the navbar buttons.

ManuelMcLure commented 2 years ago

Yeah - the popover is a sibling of the anchor tag. I'll do some experimenting and see what I can figure out.

ManuelMcLure commented 2 years ago

If I can get the container property of the popover to work I should be able to have each popover have its own setting for content visible. If I can't, I might fall back to hiding the title and making it just another line in the content, more similarly to how the original tooltip worked. But I'm not giving up yet!

ManuelMcLure commented 2 years ago

The following code almost works to make the popover a child of the <a> tag:

<a class="tasmota_button"
    href="#"
    data-container='#tasmota_button_link_0'
    data-bind="click: $root.toggleRelay,visible: $root.loginState.loggedIn(), popover: {title: $data.label, html: true, content: $root.arrSmartplugsTooltips.get(ip()+'_'+idx()), placement: 'bottom', trigger: 'hover'}, attr: { id: 'tasmota_button_link_'+$index()}"
    style="display: none;">
        <i class="icon"
            data-bind="css: [$root.arrSmartplugsStates.get(ip()+'_'+idx())(), icon(),(($root.processing().indexOf(ip()) > -1) ? 'icon-spin' : '')].join(' '), style: {color: $root.get_color($data)}"
            aria-hidden="true"></i>
    <div class="tasmota_label" data-bind="text: $data.label_extended"></div>
</a>

In fact, it does work to make all of the popovers children of the first <a> tag in the list. But I haven't found a way to make each popover be a child of the <a> tag that instantiated it.

data-container='#tasmota_button_link_'+$index()

doesn't work.

ManuelMcLure commented 2 years ago

The other option I tried was to trigger the popover from the <i> instead of the <a> - that works, but has the issue that you need to hover over the icon instead of the whole area of the button for the popover to show up. The same problem occurs if I put a <div> wrapping the <i> and trigger the popover from that.

ManuelMcLure commented 2 years ago

Since the popover shows up right after the element that created it, I can use the CSS '+' selector (select element right after the described element) to trigger the style differences.