-
Notifications
You must be signed in to change notification settings - Fork 43
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
fix: prevent invisible tooltips for the zero value of legendOpacity from the api #102
Conversation
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
return LEGEND_OPACITY_DEFAULT | ||
}, [legendOpacity]), | ||
LEGEND_OPACITY_MINIMUM | ||
) |
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.
All of these are doing the exact same thing. I would pull them out into their own hook with a useCallback
whose dependency array is contains legendOpacity
. Then each of these files would read something like:
const tooltipOpacity = useLegendOpacity(legendOpacity)
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.
Ok, will do.
Also, unrelated. A better default is a solid looking tooltip rather than a barely visible 0.2
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.
if this gets much larger we might want to pull all of this into something like useLegendSettings
. Looks great.
aa8280a
to
0d69307
Compare
The force push was to fix merge conflicts. |
The initial value from the backend api comes back as 0 for opacity, which doesn't change in props, causing legends to appear invisible. Prevent this by ensuring minimum opacity is always used.