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

feat(legend/series): add hover interaction on legend items #31

Merged
merged 21 commits into from
Feb 14, 2019

Conversation

emmacunningham
Copy link
Contributor

@emmacunningham emmacunningham commented Feb 4, 2019

This PR implements the feature (see #24) to add a hover interaction with legend items that highlights the corresponding series in the chart. Currently, this is implemented to set the opacity of the non-highlighted series to 0.25 and the highlighted series to 1. This renders like below:

bar series:
bar_legend

line series:
line_legend

area series:
area_legend

mixed series:
mixed_legend

Per the discussion below, there is also a refactoring of how specId and seriesKey are associated with a Geometry. All Geometries now have will have these properties as they are required to be able to identify a specific geometry belonging to a spec within a series.

Also, per the discussion below, as we will likely want to share some common styles across all the different geometries, while this PR only introduces the stylistic feature of changing the opacity based on a highlighted legend item, there is the ability to update these shared styles in a way that allows for re-use across geometries.

@markov00
Copy link
Member

markov00 commented Feb 6, 2019

  • Currently, this is implemented to affect opacity. Is this correct or is there another visual change that should be implemented to achieve the "highlighting" effect?

I think it's fine for now adding the opacity. We can add a border later, but I will let the design team decide

  • I assume that whatever the highlighting effect is, it should be the same for all series visualizations. So once it is settled what the highlighting effect should be, I will move the rendering logic for this to a shared utility rather than how it is now (with each geometries component handling the logic itself).

yes you are right.

  • I noticed that for both AreaGeometry and LineGeometry, the GeometryValue (which contains the specId and seriesKey, which is used to determine what should be highlighted or not) does not exist on the Geometry, but rather each of the points' PointGeometry. However, an entire Area or Line would be highlighted as we hover over each legend item, so right now to get around this, I check the first element in points on AreaGeometry and LineGeometry (for BarGeometry, the GeometryValue is directly there as value on each bar), and use that to determine if an area or line should be highlighted. This will technically work, but I'm wondering if we might want AreaGeometry and LineGeometry to also directly have value like BarGeometry to avoid having to check the first point.

Yes please, add a GeometryValue also on AreaGeometry and LineGeometry. it can be a Partial<GeometryValue> with only the specId set or we can split the GeometryValue into something like and use GeometryId on Area and Line:

interface GeometryId {
  specId: SpecId;
}
export type GeometryValue = GeometryId & {

  datum: any;
  seriesKey: any[];
}

Or another possibility is to add on the datum an array of GeometryValues? I will reduce that to the minimum required for the highlight for the moment and than we can add more data if we need it

@emmacunningham
Copy link
Contributor Author

@markov00 Thank you for the input!

See efd482d for how the necessary specId and seriesKey are being added to each type of Geometry so we don't have to access individual points.

And to address the shared styles (as well as the likelihood that design team will offer more feedback on ways to distinguish the highlighted series), see these commits: ae908ae and 647edd6

@emmacunningham emmacunningham changed the title [wip] feat(legend/series): add hover interaction on legend items feat(legend/series): add hover interaction on legend items Feb 7, 2019
@emmacunningham emmacunningham requested review from markov00 and removed request for markov00 February 7, 2019 19:08
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added few comments. Can you also add 4 stories to simulate that hover effects?

  • one only bars
  • one only lines
  • one only areas
  • one with areas, bars and lines

src/lib/series/rendering.ts Outdated Show resolved Hide resolved
src/lib/series/series_utils.ts Outdated Show resolved Hide resolved
src/state/chart_state.ts Outdated Show resolved Hide resolved
src/components/react_canvas/bar_geometries.tsx Outdated Show resolved Hide resolved
src/components/react_canvas/bar_geometries.tsx Outdated Show resolved Hide resolved
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the interaction and I see two last main (one minor) issues:

  • minor: do we always want to have the highlight on legend enabled? we can maybe add a general strategy to deal with defaults interactions like these ones (on a different PR)
  • major: I've see that the over, out listeners fires few times hovering the same element (see gif) seems that between the two elements, on the padding, the pointer pass through and a mouseout is fired between the dot and the text. I think we should check this and have a consistent mouseover-mouseout event.
    feb-11-2019 15-38-21
    -nit: do you mind adding a hover style on the legend item? an underline for the moment is fine, just to have a feedback on the hovered element.

@emmacunningham
Copy link
Contributor Author

* minor: do we always want to have the highlight on legend enabled? we can maybe add a general strategy to deal with defaults interactions like these ones (on a different PR)

👍 Opened a separate issue to track this for a new PR. (#56)

* major: I've see that the over, out listeners fires few times hovering the same element (see gif) seems that between the two elements, on the padding, the pointer pass through and a mouseout is fired between the dot and the text. I think we should check this and have a consistent mouseover-mouseout event.
  ![feb-11-2019 15-38-21](https://user-images.githubusercontent.com/1421091/52570548-f5868180-2e13-11e9-95bd-678d5c177ad7.gif)

Using mouseenter/leave fixes this. See 3fe141a

legend_item_hover_event

  -nit: do you mind adding a hover style on the legend item? an underline for the moment is fine, just to have a feedback on the hovered element.

Yes, I had ended up handling this in the PR for the legend click feature, which you can see here: https://github.com/elastic/elastic-charts/pull/51/files#diff-f6c762a3a7e1c32b9339b3e7cdc3fff1R101. I cherry-picked the hover style (669da2a).

@emmacunningham
Copy link
Contributor Author

@markov00 One more thing: I think there will be some big conflicts with the new Theme structure from #44, so it is probably best first to merge that one and then I can resolve those conflicts here (and in the other legend item PR that is open, which is marked WIP until this one is merged in because that was branched off of this one).

@emmacunningham emmacunningham merged commit c56a252 into elastic:master Feb 14, 2019
markov00 pushed a commit that referenced this pull request Feb 14, 2019
# [1.1.0](v1.0.2...v1.1.0) (2019-02-14)

### Features

* **legend/series:** add hover interaction on legend items ([#31](#31)) ([c56a252](c56a252)), closes [#24](#24)
@markov00
Copy link
Member

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Feb 14, 2019
@markov00 markov00 mentioned this pull request Mar 27, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants