-
Notifications
You must be signed in to change notification settings - Fork 47
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(highlight): Render promoter tooltip #557
Conversation
@@ -0,0 +1,94 @@ | |||
import * as React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a convention for folder names? Seems like most folders are lowercased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my bad. I used PascalCase for the component folder names due to old habit. They should be hyphenated lowercase, instead. Seems like something to change in a separate PR, though.
const { x, y, height, width } = rect; | ||
|
||
const reference = { | ||
getBoundingClientRect: () => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was going to ask if you could use the DOMRect constructor but IE (https://developer.mozilla.org/en-US/docs/Web/API/DOMRect/DOMRect)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be worth a simple polyfill so we can use the constructor style? We use it in several places now, iirc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I will submit a separate PR to address this.
874cd84
to
adbe203
Compare
const { x, y, height, width } = rect; | ||
|
||
const reference = { | ||
getBoundingClientRect: () => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be worth a simple polyfill so we can use the constructor style? We use it in several places now, iirc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please make sure to regression test the existing region functionality, especially given the changes to popup arrows and the BUIE upgrade.
No description provided.