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

fix: prevent invisible tooltips for the zero value of legendOpacity from the api #102

Merged
merged 2 commits into from
Sep 24, 2020

Conversation

TCL735
Copy link
Contributor

@TCL735 TCL735 commented Sep 24, 2020

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.

@TCL735 TCL735 requested review from hoorayimhelping, alexpaxton and a team September 24, 2020 17:35
Copy link
Contributor

@alexpaxton alexpaxton left a 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
)
Copy link
Contributor

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)

Copy link
Contributor Author

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

@TCL735 TCL735 requested a review from 121watts September 24, 2020 19:58
Copy link
Contributor

@121watts 121watts left a 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.

@TCL735 TCL735 force-pushed the fix_legend_opacity_zero_value branch from aa8280a to 0d69307 Compare September 24, 2020 20:34
@TCL735
Copy link
Contributor Author

TCL735 commented Sep 24, 2020

The force push was to fix merge conflicts.

@TCL735 TCL735 merged commit b7dd3ed into master Sep 24, 2020
@TCL735 TCL735 deleted the fix_legend_opacity_zero_value branch September 24, 2020 21:59
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