Skip to content

feat(annotations): add onClickHandler for annotations#1293

Merged
rshen91 merged 64 commits intoelastic:masterfrom
rshen91:click-listener
Sep 1, 2021
Merged

feat(annotations): add onClickHandler for annotations#1293
rshen91 merged 64 commits intoelastic:masterfrom
rshen91:click-listener

Conversation

@rshen91
Copy link
Copy Markdown
Contributor

@rshen91 rshen91 commented Aug 10, 2021 •
edited by nickofthyme
Loading

Summary

The onAnnotationClick prop is now available in the Settings component to listen to click events on LineAnnotation (marker only) and RectAnnnotation.

Details

Example code snippet from the new storybook example in annotations > rects > click-handle-rect-annotation:

 export const Example = () => {
  const onAnnotationClick = action('onAnnotationClick');

  return (
    <Chart>
      <Settings baseTheme={useBaseTheme()} onAnnotationClick={onAnnotationClick} />
      <RectAnnotation
        id="rect1"
        dataValues={[
          {
            coordinates: {
              x0: 0,
              x1: 1,
              y0: 0,
              y1: 4,
            },
            details: 'details about this annotation',
          },
        ]}
        style={{ fill: 'red' }}
      />
      <RectAnnotation
        id="rect2"
        dataValues={[
          {
            coordinates: {
              x0: 1,
              x1: 2,
              y0: 1,
              y1: 5,
            },
            details: 'details about this other annotation',
          },
        ]}
        style={{ fill: 'blue' }}
      />
      <LineAnnotation
        id="line_annotation"
        domainType={AnnotationDomainType.XDomain}
        dataValues={[{ dataValue: 2, details: `detail-${0}` }]}
        marker={<Icon type="alert" />}
        markerPosition={Position.Top}
      />
...

Issues

Closes #1211 request from the ML team

Checklist

  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@rshen91 rshen91 changed the title feat(annotations): add onClickHandler for annotationss feat(annotations): add onClickHandler for annotations Aug 10, 2021
@monfera
Copy link
Copy Markdown
Contributor

monfera commented Aug 18, 2021

What's the approach for the mouse capture area padding? For example, the actual line of a line annotation is very thin, and Fitts law needs significant meat for being able to reliably select. Then again, there may be some tightly spaced lines, in which case the generous padding is constrained so that the midline between two nearby parallel lines determines if this or that line gets selected (otherwise the capture zone of one line can occlude the other line). So, a one-dimensional Voronoi version of this 2D example—or the Voronoi picker @nickofthyme added to charts—at least conceptually, and the 1D Voronoi logic can be extracted into a common directory for later reuse.

Capture zone padding gets more complicated with mixed or overlapping annotations: both horizontal and vertical lines; lines over rectangles; overlapping rectangles; annotation over the text of some other annotation; annotation over bars, data point markers or other interactive elements.

So, just curious about what we have as current goals for this, maybe on the original issue, or on a separate issue that goes into this kind of detail (or discussed as part of the PR)

@rshen91
Copy link
Copy Markdown
Contributor Author

rshen91 commented Aug 18, 2021

What's the approach for the mouse capture area padding? For example, the actual line of a line annotation is very thin, and Fitts law needs significant meat for being able to reliably select. Then again, there may be some tightly spaced lines, in which case the generous padding is constrained so that the midline between two nearby parallel lines determines if this or that line gets selected (otherwise the capture zone of one line can occlude the other line). So, a one-dimensional Voronoi version of this 2D example—or the Voronoi picker @nickofthyme added to charts—at least conceptually, and the 1D Voronoi logic can be extracted into a common directory for later reuse.

Capture zone padding gets more complicated with mixed or overlapping annotations: both horizontal and vertical lines; lines over rectangles; overlapping rectangles; annotation over the text of some other annotation; annotation over bars, data point markers or other interactive elements.

So, just curious about what we have as current goals for this, maybe on the original issue, or on a separate issue that goes into this kind of detail (or discussed as part of the PR)

Hey! I might be misinterpreting, but this only should make the line annotation marker clickable if passed an onAnnotationClick listener not the whole line.

@rshen91 rshen91 added :annotation Annotation (line, rect, text) related issue :xy Bar/Line/Area chart related labels Aug 18, 2021
@rshen91 rshen91 marked this pull request as ready for review August 18, 2021 19:50
@monfera
Copy link
Copy Markdown
Contributor

monfera commented Aug 19, 2021

this only should make the line annotation marker clickable if passed an onAnnotationClick listener not the whole line

Thanks Rachel! It wasn't clear to me, as the issue (to which I need to compare the PR) mentions annotations, and specifically the line and rectangle annotations. Could the scope be added to the gh issue, ie. it's just the annotation markerand not the entire rectangle or line, so I only review things in the PR that are agreed to change.

I guess that for the annotation marker, it'll stay as it is, basically no padding. @markov00 do we want to add padding to the markers, and are there signs of needs for capture events on the lines/rectangles?

nickofthyme
nickofthyme requested changes Aug 19, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Looking good! I made a few suggestions along with some questions. Ping me if something doesn't make sense.

@rshen91
Copy link
Copy Markdown
Contributor Author

rshen91 commented Aug 25, 2021

In aa8c050 I updated the failing vrts - these were exclusively screenshots that had line annotations and had slight pixel changes

nickofthyme
nickofthyme requested changes Aug 25, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Playing with the pr locally, there is a pointer that is not clickable

@rshen91
Copy link
Copy Markdown
Contributor Author

rshen91 commented Aug 25, 2021

Thank you, good catch. I was hoping there was some easy fix with the get_cursor_pointer selector but it wasn't working. This fix in commit 828ab13 works - let me know what you think. Thanks

@rshen91 rshen91 requested a review from nickofthyme August 25, 2021 19:54
@rshen91 rshen91 requested a review from markov00 August 26, 2021 16:27
nickofthyme
nickofthyme requested changes Aug 30, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

The cursor pointer logic does not match the actual logic. The onAnnotationClick event should NOT fire over element geometry. Also check how this acts with line charts when clicking points above the rect annotations.

Screen.Recording.2021-08-30.at.02.13.54.PM.mp4

@rshen91
Copy link
Copy Markdown
Contributor Author

rshen91 commented Aug 31, 2021

The cursor pointer logic does not match the actual logic. The onAnnotationClick event should NOT fire over element geometry. Also check how this acts with line charts when clicking points above the rect annotations.

Screen.Recording.2021-08-30.at.02.13.54.PM.mp4

Please see 4c07301 for the fix thank you for catching this!

@rshen91 rshen91 requested a review from nickofthyme August 31, 2021 18:39
nickofthyme
nickofthyme approved these changes Sep 1, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Looks great @rshen91! Great work.

@rshen91 rshen91 merged commit 48198be into elastic:master Sep 1, 2021
@smith smith mentioned this pull request Sep 3, 2021
Open
nickofthyme pushed a commit that referenced this pull request Sep 13, 2021
@semantic-release-bot
chore(release): 35.0.0 [skip ci]
b82f724 
# [35.0.0](v34.2.1...v35.0.0) (2021-09-13)

### Bug Fixes

* **a11y:** restore focus after popover close with color picker ([#1272](#1272)) ([0c6f945](0c6f945)), closes [#1266](#1266) [#935](#935)
* **build:** fix license in package.json ([#1362](#1362)) ([d524fdf](d524fdf))
* **deps:** update dependency @elastic/eui to ^37.5.0 ([#1341](#1341)) ([fb05c98](fb05c98))
* **deps:** update dependency @elastic/eui to ^37.6.1 ([#1359](#1359)) ([2ae90ce](2ae90ce))
* **deps:** update dependency @elastic/eui to ^37.7.0 ([#1373](#1373)) ([553b6b0](553b6b0))
* **heatmap:** filter out tooltip picked shapes in x-axis area ([#1351](#1351)) ([174047d](174047d)), closes [#1215](#1215)
* **heatmap:** remove values when brushing only over axes ([#1364](#1364)) ([77ff8d3](77ff8d3))

### Features

* **annotations:** add onClickHandler for annotations ([#1293](#1293)) ([48198be](48198be)), closes [#1211](#1211)
* **heatmap:** add text color contrast to heatmap cells ([#1342](#1342)) ([f9a26ef](f9a26ef)), closes [#1296](#1296)
* **heatmap:** reduce font size to fit label within cells ([#1352](#1352)) ([16b5546](16b5546))
* **xy:** mutilayer time axis step 1 ([#1326](#1326)) ([867b1f5](867b1f5))

### BREAKING CHANGES

* **xy:** - feat: removes the axis deduplication feature
- fix: `showDuplicatedTicks` causes a duplication check on the actual axis tick label (possibly yielded by `Axis.tickLabel` rather than the more general `tickFormat`)
* **heatmap:** the `config.label.fontSize` prop is replaced by `config.label.minFontSize` and `config.label.maxFontSize`. You can specify the same value for both properties to have a fixed font size. The `config.label.align` and `config.label.baseline` props are removed from the `HeatmapConfig` object.
@nickofthyme
Copy link
Copy Markdown
Collaborator

nickofthyme commented Sep 13, 2021

🎉 This PR is included in version 35.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Sep 13, 2021
@markov00 markov00 mentioned this pull request Oct 5, 2021
Merged
wkrause13 pushed a commit to wkrause13/elastic-charts that referenced this pull request Oct 15, 2022
@rshen91
feat(annotations): add onClickHandler for annotations (elastic#1293)
03ba632 
Closes elastic#1211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickofthyme nickofthyme nickofthyme approved these changes

@markov00 markov00 Awaiting requested review from markov00

Assignees

No one assigned

Labels

:annotation Annotation (line, rect, text) related issue released Issue released publicly :xy Bar/Line/Area chart related

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Charts] Annotations should have click handler

3 participants

@rshen91 @monfera @nickofthyme