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(react-ui): Fix LegendProportion radius scale #877

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

donmccurdy
Copy link
Member

Description

Shortcut: Story 415171, URL redacted.

LegendProportion displays the legend for point scaling, and sometimes calculates the scale incorrectly. In the image below, note that the 'max' is smaller than the second number, and the 'min' is larger than the third.

The cause is a typo, we should be using max - min instead of max + min to calculate the interval over which the two middle steps are interpolated. With this bug, the steps were calculated correctly only when min was zero.

Additionally, the visual sizes of the circles in the legend panel are hard-coded to [3, 6, 9, 12], representing two evenly-spaced steps between the min and max. However, our implementation would have generated different steps given min=3, max=12 as input, because it divided the interval by 4, not 3.

before after
legend_range_bug Screenshot 2024-06-11 at 12 39 00 PM

Type of change

  • Fix

Acceptance

Please describe how to validate the feature or fix

  1. Link C4R into Builder

  2. Create a new map

  3. Add SQL query

    SELECT * FROM `carto-demo-data.demo_tables.cell_towers_worldwide` AS table
    WHERE (table.lat - 36.744874) < 5 AND (table.lon - 3.195371) < 5;
  4. Set point layer to style radius by lat column

  5. Open legend, observe steps are correctly spaced

@donmccurdy donmccurdy requested a review from a team June 11, 2024 16:48
Copy link

Pull Request Test Coverage Report for Build 9469631381

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.009%) to 71.215%

Totals Coverage Status
Change from base Build 9414264087: -0.009%
Covered Lines: 2804
Relevant Lines: 3627

💛 - Coveralls

1 similar comment
Copy link

Pull Request Test Coverage Report for Build 9469631381

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.009%) to 71.215%

Totals Coverage Status
Change from base Build 9414264087: -0.009%
Covered Lines: 2804
Relevant Lines: 3627

💛 - Coveralls

Copy link

github-actions bot commented Jun 11, 2024

Visit the preview URL for this PR (updated for commit 4916345):

https://cartodb-fb-storybook-react-dev--pr877-donmccurdy-fix-l-5w4wmit6.web.app

(expires Wed, 19 Jun 2024 14:32:35 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 517cc4d31d7e09cf277774e034094b67c301cd4c

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 9469631381

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.009%) to 71.215%

Totals Coverage Status
Change from base Build 9414264087: -0.009%
Covered Lines: 2804
Relevant Lines: 3627

💛 - Coveralls

Copy link
Contributor

@zbigg zbigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

Pull Request Test Coverage Report for Build 9484450119

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 71.22%

Totals Coverage Status
Change from base Build 9414264087: -0.004%
Covered Lines: 2805
Relevant Lines: 3628

💛 - Coveralls

2 similar comments
Copy link

Pull Request Test Coverage Report for Build 9484450119

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 71.22%

Totals Coverage Status
Change from base Build 9414264087: -0.004%
Covered Lines: 2805
Relevant Lines: 3628

💛 - Coveralls

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 9484450119

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 71.22%

Totals Coverage Status
Change from base Build 9414264087: -0.004%
Covered Lines: 2805
Relevant Lines: 3628

💛 - Coveralls

@donmccurdy donmccurdy merged commit 2e7cc6a into master Jun 12, 2024
2 checks passed
@donmccurdy donmccurdy deleted the donmccurdy/fix-LegendProportion-scale branch June 12, 2024 14:39
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