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

update plotly.js to v2.4.1 #1735

Merged
merged 15 commits into from
Sep 2, 2021
Merged

update plotly.js to v2.4.1 #1735

merged 15 commits into from
Sep 2, 2021

Conversation

alexcjohnson
Copy link
Collaborator

@alexcjohnson alexcjohnson commented Aug 30, 2021

FYI I specifically did NOT commit build artifacts because @HammadTheOne is working on removing these from the repo entirely (oh there is is, #1734). AFAICT the tests all rebuild all the components anyway.

>
<span
id={id}
className={`hover-content ${props.className}`}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discovered while writing the test, but anyway users will expect props.id to be in the DOM. I put it on the content <span>, because this is where props.className is applied, and that's mostly where you probably want your styles applied. Makes it a little awkward to style the container, but because of the pseudo-elements making the triangle this is discouraged anyway, and we have special props for bg and border colors there already.

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

This all looks reasonable to me, assuming others have tried out the tooltip component and are happy with the end user behavior.
💃

@alexcjohnson alexcjohnson merged commit cf75745 into dev Sep 2, 2021
@alexcjohnson alexcjohnson deleted the plotlyjs-2.4.1 branch September 2, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants