Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

Conversation

@kristw
Copy link
Collaborator

@kristw kristw commented Mar 18, 2020

🏆 Enhancements

The deck gl charts used to rely on props setTooltip from superset app to display tooltip.
This PR add support for the polygon chart to handle its own tooltip independently from superset app.

From storybook
image

Also tested in Superset
image

@kristw kristw requested a review from a team as a code owner March 18, 2020 20:43
Copy link

@williaster williaster left a comment

Choose a reason for hiding this comment

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

LGTM overall, couple small questions you're prob already on top of

{children}
</div>
<>
<Tooltip tooltip={tooltip} />

Choose a reason for hiding this comment

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

should this be rendered on top of the div below? (not sure what z-index is like etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. Will try moving it down.

import { StaticMap } from 'react-map-gl';
import DeckGL from 'deck.gl';
// eslint-disable-next-line import/extensions
import Tooltip from './components/Tooltip';

Choose a reason for hiding this comment

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

any reason to not just add .tsx? (def a style thing 🤷‍♂ )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will cause file not found issue after transpile because the code will remain

import Tooltip from './components/Tooltip.tsx';

but Tooltip.tsx also became Tooltip.js through transpilation already.

return this.props.layers;
}

setTooltip = tooltip => {

Choose a reason for hiding this comment

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

do other components call these setTooltip fn()s? can't find references in the file so wanted to verify

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, AnimatedDeckGLContainer and CategoricalDeckGLContainer contain DeckGLContainer and call it via ref.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants