-
Notifications
You must be signed in to change notification settings - Fork 3
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(20649): Always show numbers in Bivariate hex popup #964
fix(20649): Always show numbers in Bivariate hex popup #964
Conversation
WalkthroughThe changes involve simplifying the rendering logic in the BivariateRenderer by removing a feature flag-based condition for displaying values in a map tooltip. The modification eliminates the conditional check that previously controlled the visibility of x and y values, ensuring that the values are always passed to the tooltip component regardless of previous feature flag settings. Additionally, the MapHexTooltip component now requires the values prop, enforcing stricter usage. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Bundle size diff
|
Preview environments for this PR:
These previews are automatically updated with each commit. Note: After a new deployment, it may take a few minutes for the changes to propagate and for caches to update. During this time, you might experience temporary loading issues or see an older version of the app. If the app fails to load, please wait a few minutes and try again. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
=======================================
Coverage 10.04% 10.04%
=======================================
Files 668 668
Lines 29398 29395 -3
Branches 1235 1235
=======================================
Hits 2954 2954
+ Misses 25915 25912 -3
Partials 529 529
☔ View full report in Codecov by Sentry. |
Bundle ReportChanges will decrease total bundle size by 78 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
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 you 100% sure that values
always should be presented in such tooltip, you can also remove optional from props definition of MapHexTooltipProps
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/MapHexTooltip/MapHexTooltip.tsx
(2 hunks)
🔇 Additional comments (1)
src/components/MapHexTooltip/MapHexTooltip.tsx (1)
33-33
: Breaking change:values
prop is now required.Making the
values
prop required is a breaking change that will affect all consumers of this component. While this aligns with the PR objective to always show numbers, we should verify that all usage sites provide this prop.Let's verify all component usages:
✅ Verification successful
Making
values
prop required is safeThe only usage of MapHexTooltip in the codebase (in BivariateRenderer.tsx) already provides the required
values
prop with both x and y values. This change aligns with the existing implementation and won't cause any breaking changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all files that import or use MapHexTooltip rg -l "MapHexTooltip" # Search for MapHexTooltip usage patterns ast-grep --pattern 'MapHexTooltip $$$'Length of output: 183
Script:
#!/bin/bash # Look for MapHexTooltip JSX usage with context rg -U "MapHexTooltip.*?>" -A 5 -B 5 src/core/logical_layers/renderers/BivariateRenderer/BivariateRenderer.tsx # Alternative search using ast-grep for JSX element ast-grep --pattern '<MapHexTooltip $$$>'Length of output: 154
Script:
#!/bin/bash # Check import statement rg "import.*MapHexTooltip" src/core/logical_layers/renderers/BivariateRenderer/BivariateRenderer.tsx # Get more context around any MapHexTooltip mention rg -A 10 -B 10 "MapHexTooltip" src/core/logical_layers/renderers/BivariateRenderer/BivariateRenderer.tsx # Try to find any component props spread rg -A 5 -B 5 "{\s*\.\.\..*}" src/core/logical_layers/renderers/BivariateRenderer/BivariateRenderer.tsxLength of output: 1971
https://kontur.fibery.io/Tasks/Task/Show-numbers-in-bivariate-popup-by-default-20649
Summary by CodeRabbit
values
a required property in map hex tooltip component, ensuring consistent value display.