humangeo / leaflet-dvf

Leaflet Data Visualization Framework
MIT License
689 stars 153 forks source link

Customizing tooltips #102

Open joakimkejser opened 7 years ago

joakimkejser commented 7 years ago

Hi,

First of all, thanks for a great library!

I have some questions regarding tooltips:

Sometimes, I would like to use displayOptions and a data property to customize a visualization, but I do not want to show the data property in the tooltip - is there a way to avoid that?

Is there an easier way to change the "title" of the tooltips (the one above the legend)? I have managed to do so for a MapMarkerDataLayer, where per default the lat/lon is showed, by using the custom location mode and changing the text property of the location object. However, this seems like a bit of a workaround.

Is it possible to disable the legend in the tooltips?

Finally, is it possible to completely customize the content of the tooltip, or do I need to implement my own with the onEachRecord function?

Regards, Joakim Kejser

sfairgrieve commented 7 years ago

Thanks @joakimkejser. Great questions, which should lead to some new features in the future. You should be able to disable the tooltips altogether by setting showLegendTooltips to false in the options you pass in to the DataLayer. You can exclude properties you've called out in displayOptions by setting excludeFromLegend to true under the options associated with a given display option.

In terms of the legend title, I'm assuming you're referring specifically to the location text value that's displayed? The only way to set this now is through the workaround you mentioned. I agree that this should be simpler to customize and will add this to the list of updates that need to be made.

It's not easy at the moment to customize the tooltip content, but I'll add that to the list of updates as well.

joakimkejser commented 7 years ago

@sfairgrieve, thanks for getting back to me.

excludeFromLegend does indeed exclude the given display option from the legend, but not from the tooltip. This behavior is demonstrated in the "DC Metro Bus Locations" example. I can however also imagine examples where you would want to exclude a displayOptions from the tooltip but include it in the legend (say you dynamically scale the radius/weight with an arbitrary property not meant for display in the tooltip).

_getLegend take the excludeFromLegend option into account, but _bindMouseEvents does not filter legendDetails before passing it on to L.LegendIcon if I have interpreted the code correctly. Maybe there should be an excludeFromTooltip option?

I am specifically referring to the location text value that is displayed, yes. In relation to that, the L.Graph layer disregards the location.text value of the to and from location retrieved and instead displays the fromField and toField value. This works great in the airport example but leaves no room for customization. I believe text: fromValue + ' - ' + toValue should instead be text: fromLocation.text + ' - ' + toLocation.text here, which would yield the same result in the airport example.

Another option I would like to see is the ability to also hide the legend box (the colored/styled little icon) in the tooltips. I find them very useful in some cases, but when using a MapMarkerDataLayer unstyled or with very few color options, I would like to exclude them.

Once again, thank you for your work on this project - it is immensely useful.

Let me know if there is anything I can do to help.

Regards, Joakim

sfairgrieve commented 7 years ago

You're right. Great points/suggestions. I think these are all things that should be configurable.

You should be able to hide the legend boxes using CSS. Something like:

div.leaflet-div-icon div.legend-box {
   display: none;
}

Let me know if that doesn't work.

I'm definitely open to contributions, so if you feel like implementing any of these suggestions, I'd be happy to accept pull requests. I'll let you know when I start working on these features once I get some free time (hopefully soon).

Scott

sfairgrieve commented 7 years ago

@joakimkejser After reviewing the code, I also forgot that this option existed, but you can pass in an option called dynamicOptions that is a function that takes a record as input and should return an object with a layerOptions property (key/value pairs for styling the legend box) and a legendDetails property for what fields/values to display. So:

var layer = new L.DataLayer(data, {
...
dynamicOptions: function (record) {
    return {
       layerOptions: {
          color: 'red'
       },
       legendDetails: {
          field1: value1
       }
    };
}
...
});

The _getDynamicOptions function of L.DataLayer is the default implementation of this. Can you give this a try and see if it works? I think this would essentially solve some of your issues, although it's more complex than the options you suggested. I'm looking into adding those in as well.

joakimkejser commented 7 years ago

@sfairgrieve Yes, that does indeed work with a small change:

dynamicOptions: function(record) {
    opt = layer._getDynamicOptions(record);
    return {
        layerOptions: opt.layerOptions,
        legendDetails: {
            "whatever": {
                 "name": "Name",
                 "value": record.value
            }
        }
    }
}

I called the original function to avoid having to define all my layerOptions again, but this is also a nice way to allow complete control over those. The legendDetails need a "name" and "value" for it to work.

Is there a way to call _getDynamicOptions without referencing my layer instance?

Thanks! This solves my problem for now. I will see if I can help with some of the other options when I get time.

sfairgrieve commented 7 years ago

Great! Yeah, I just noticed that dynamicOptions is not being passed the DataLayer instance as the context (I'll fix that). Once I fix that, you should be able to call this._getDynamicOptions(record). Saw your pull request as well and will try to accept that tonight. Your help and input is definitely appreciated!

joakimkejser commented 7 years ago

@sfairgrieve the pull request is just a minor change, so no hurries. Asesome, the context will be very useful in dynamicOptions.

I have stumbled upon another issue related to tooltips. If showLegendTooltips is set to false, setHighlight and unsetHighlight cannot be used to create a hover effect, as these are bound in the _bindMouseEvents which is only called if showLegendTooltips is true.

A solution could be to move the check on showLegendTooltips to the _bindMouseEvents, as to only exclude tooltip related stuff and leave in the setHighlight and unsetHighlight bit.

A workaround for now is to leave the showLegendTooltips to true, and do:

showLegendTooltips: true,
tooltipOptions: {
  iconSize: new L.Point(0,0),
  iconAnchor: new L.Point(-5000,5000)
}

It is a bit of a hack, but it gets the tooltip out of the way.

sfairgrieve commented 7 years ago

@joakimkejser I pushed some updates to master and 1.0dev that address some of your suggestions. Can you give these updates a try when you get a chance? I did some testing, but I'm hoping you can validate things to make sure I didn't miss anything. I may also break out your suggestions into separate issues, so I can track them better.

joakimkejser commented 7 years ago

@sfairgrieve Thanks for working on this. When I load in 1.0dev, everything works fine, but master gives me an error. I think I've tracked it down to some issue with L.ArcedPolyline where the implementation of initialize is different between master and 1.0dev. To be fair, I previously used the distribution used in the examples: http://humangeo.github.io/leaflet-dvf/dist/leaflet-dvf.js, which shares the implementation with the 1.0dev branch. So it is probably not related to your recent changes, and my code might have been faulty all along. I get the following error:

Uncaught TypeError: Cannot read property 'call' of undefined
    at e.initialize (leaflet-dvf.js:7164)
    at new e (leaflet.js:5)
    at e.ARC (leaflet-dvf.js:8045)
    at e._getLocation (leaflet-dvf.js:8082)
    at e._loadRecords (leaflet-dvf.js:5402)
    at e.addData (leaflet-dvf.js:5517)
    at e.initialize (leaflet-dvf.js:5181)
    at new e (leaflet.js:5)
    at HTMLDocument.<anonymous> (report.html:804)
    at i (jquery.min.js:2)

where at HTMLDocument.<anonymous> (report.html:804) is var layer = new L.Graph(data, options);

Maybe you have some inputs on that?

I'll take a look at the specific features you have implemented tomorrow and validate. Just to clarify, from reading the code I understand that you have added/fixed the following:

Have I missed anything?

Great idea seperating the issues - sorry for pouring them all into one issue. I can try and create seperate issues and reference the fixes if you'd like?

sfairgrieve commented 7 years ago

Thanks @joakimkejser. That makes sense. master is setup to work with Leaflet 0.7.x and 1.0dev is setup to work with Leaflet 1.0.x. At some point in the near future, I'll switch master to be the 1.0.x compatible version and ween off any updates related to the 0.7.x version. It's tough trying to support both codebases.

The changes you've listed are right. I also updated the code so that the options.dynamicOptions function gets passed this (the DataLayer instance) as the context, so you should be able to call this._getDynamicOptions rather than layer._getDynamicOptions. I'll start adding any of the suggestions I missed as issues tomorrow and begin trying to address them.

joakimkejser commented 7 years ago

@sfairgrieve I tested all of the above against the 1.0dev branch, and it works as intended! Reading through our discussions above, the enhancements related to tooltips that are still missing are:

With that in place, I think we have achieved completely customizable tooltips!

I might have some additional enhancement proposals for L.Graph, but I'll open separate issues for that when I have explored/formalized my thoughts on that.

Thanks for spending time on this!

sfairgrieve commented 7 years ago

Thanks for validating. I'll plan to add those additional features in the next few days. Any feedback/improvements you have for L.Graph will be much appreciated.

sfairgrieve commented 7 years ago

@joakimkejser I pushed some updates to the feature/custom-tooltips branch, if you want to check it out. This should address #104 and #105.

For #104:

Under tooltipOptions you can include a getTooltip function that takes the record, legendDetails, and layerOptions and returns an L.DivIcon instance.

For #105:

Under tooltipOptions you can include a hideLegendBox property with a value of true to hide the legend box.

Let me know what you think when you get a chance.

joakimkejser commented 7 years ago

Hi @sfairgrieve , Thanks for you continued effort to improve the tooltips.

After remembering to update the css, the hideLegendBox property in the tooltipOptions on the 'feature/custom-tooltips' branch works exactly as expected, solving #105.

I will test and explore the getTooltip function later.

Cheers.

joakimkejser commented 7 years ago

Hi @sfairgrieve ,

The getTooltip in the tooltipOptions function works, and does indeed overwrite the complete tooltip! This is great!

I discovered a possible side effect of the changes. Using L.PieChartDataLayer (or any ChartDataLayer i assume), I now how to specify showLegendTooltips: true to get tooltips. This is a change from before, but it makes sense as L.ChartDataLayer specifies this as default:

options: {
            showLegendTooltips: false
        },

Furthermore, I seem to have two additional issues:

I can fix the mouserOver/Out issues by adding:

      setHighlight: function(options) {
        return options;
      },
      unsetHighlight: function(options) {
        return options;
      }

but then I lose the nice exaggerations of the slices on hover.

Can you replicate these issues or is it just me? I haven't had the time to read through the code thoroughly, so I'm sorry if I've missed something obvious.

sfairgrieve commented 7 years ago

Thanks for testing this out! I'll investigate and see if I encounter the same issues.

sfairgrieve commented 7 years ago

I see the issue. This is a side effect of removing the if statement around _bindMouseEvents a few commits back. Before, if showLegendTooltips was false, the DataLayer would not call _bindMouseEvents on child layers. Now, _bindMouseEvents is being called whether showLegendTooltips is true or false, which is preventing the child ChartMarker from getting that hover event. I'll try to put in a fix for this soon. The other issue this brings up is that ChartMarkers have their own tooltips, which the DataLayer settings don't affect - need to think about a way of fixing this/allowing customization that doesn't require duplicating too much code.

joakimkejser commented 7 years ago

@sfairgrieve, your recent changes in the feature/custom-tooltips branch seems to have solved my issues. I'm aware that there still might be some customization issues - haven't checked any of that.

vladislav-svetailo commented 3 years ago

No one of methods, which described above, didn't work for me. I didn't meet other explanation or any option in docs. In code I even didn't meet those options, which mentioned above. So, I put some changes in PieChartMarker section in leaflet-dvf.markers.js:

if(options.showTooltipOnChartPart==true) { this._bindMouseEvents(bar); }

and set option options.showTooltipOnChartPart to false. This helps me. Maybe I didn't understand something? I used version 1.0dev.