-
Notifications
You must be signed in to change notification settings - Fork 10
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
Flip tooltip position when its rendered beyond screen plot #15
base: master
Are you sure you want to change the base?
Conversation
48ce3af
to
2ab16cd
Compare
2ab16cd
to
7c4c149
Compare
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.
Thank you for the PR! 🎉 It looks (and works) awesome! I've added some code-style comments, could you take care of that? 🙏 😉
Before merge, I'd like to add tests (I'm currently working on it) to be sure that everything works fine. The package is used in many projects and we must be sure that nothing will break. 😉
constructor(props) { | ||
super(props); | ||
this.containerRef = React.createRef(); | ||
} |
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.
We can avoid declaring a constructor by assigning containerRef
as a class property (see the state
property). 😉 This approach is consistent with the current code style (IMO it's clearer and more convinient). Can you change it? 😄
const screenWidth = window.innerWidth; | ||
const screenHeight = window.innerHeight; |
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.
Can you destructure these properties?
const screenWidth = window.innerWidth; | |
const screenHeight = window.innerHeight; | |
const { innerWidth: screenWidth, innerHeight: screenHeight } = window; |
const containerWidth = this.containerRef.current.clientWidth; | ||
const containerHeight = this.containerRef.current.clientHeight; |
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.
These too, please? 😉
const containerWidth = this.containerRef.current.clientWidth; | |
const containerHeight = this.containerRef.current.clientHeight; | |
const { clientWidth: containerWidth, clientHeight: containerHeight } = this.containerRef.current; |
Description
Render tooltip from the opposite side of mouse (using same offset but in opposite direction) when mouse is positioned so close to screen edge that tooltip is rendered beyond the plot hiding it's content.
Before (mouse cursor does not appear on screenshot):
After: