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.

Sign up for GitHub

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

Jump to bottom

[feat] Add d3 legend formatting for Arc, Polygon and Scatter deck.gl maps #7951

Merged
merged 3 commits into from
Aug 11, 2019
Merged

[feat] Add d3 legend formatting for Arc, Polygon and Scatter deck.gl maps #7951

merged 3 commits into from
Aug 11, 2019

Conversation

thunter009
Copy link
Contributor

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Adds d3 legend formatting to Arc, Polygon, and Scatter deck.gl maps

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE
image

AFTER
image

TEST PLAN

Visual inspection

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

villebro
villebro reviewed Aug 1, 2019
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Made a few minor comments

superset/assets/package.json Outdated
@@ -89,6 +89,7 @@
"classnames": "^2.2.5",
"d3-array": "^1.2.4",
"d3-color": "^1.2.0",
"d3-format": "^1.3.2",
Copy link
Member

Choose a reason for hiding this comment

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

I believe you should use this instead: https://github.com/apache-superset/superset-ui/tree/master/packages/superset-ui-number-format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

superset/assets/src/visualizations/Legend.jsx Outdated
@@ -18,24 +18,52 @@
*/
import React from 'react';
import PropTypes from 'prop-types';
import { format } from 'd3-format';
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

superset/assets/src/visualizations/Legend.jsx Outdated

}

formatCatregoryLabel(k) {
Copy link
Member

Choose a reason for hiding this comment

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

Typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@etr2460
Copy link
Member

etr2460 commented Aug 6, 2019

Did you include an npm update as part of this pr? I think it would be better to split such a large change in deps to a separate PR if that was intended

@codecov-io
Copy link

codecov-io commented Aug 9, 2019 •
edited
Loading

Codecov Report

Merging #7951 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7951      +/-   ##
==========================================
- Coverage   65.58%   65.54%   -0.04%     
==========================================
  Files         469      469              
  Lines       22461    22474      +13     
  Branches     2438     2441       +3     
==========================================
  Hits        14730    14730              
- Misses       7611     7624      +13     
  Partials      120      120
Impacted Files Coverage Δ
...et/assets/src/explore/controlPanels/DeckScatter.js 0% <ø> (ø) ⬆️
...sualizations/deckgl/CategoricalDeckGLContainer.jsx 0% <ø> (ø) ⬆️
superset/assets/src/explore/controls.jsx 42.85% <ø> (ø) ⬆️
...c/visualizations/deckgl/layers/Polygon/Polygon.jsx 0% <ø> (ø) ⬆️
superset/assets/src/visualizations/Legend.jsx 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ab8e15...3b7d4d9. Read the comment docs.

@thunter009
Copy link
Contributor Author

Did you include an npm update as part of this pr? I think it would be better to split such a large change in deps to a separate PR if that was intended

I did not but switched the dependency from d3-format to superset-ui as suggested by @villebro. Everything should be good to go now.

@villebro
Copy link
Member

Are the package-lock.json changes still necessary?

@villebro villebro closed this Aug 10, 2019
@villebro villebro reopened this Aug 10, 2019
@villebro
Copy link
Member

This LGTM but I don't feel comfortable merging frontend PRs; @etr2460 or @mistercrunch can you take a look?

@mistercrunch mistercrunch merged commit 2476814 into apache:master Aug 11, 2019
@villebro villebro mentioned this pull request Aug 11, 2019
Merged
2 tasks
@villebro villebro added the v0.34 label Sep 8, 2019
@mistercrunch mistercrunch added 🍒 0.34.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@thunter009 @villebro
[feat] Add d3 legend formatting for Arc, Polygon and Scatter deck.gl …
64d23a0 
…maps (apache#7951)

* Legend formatting for Arc, Polygon and Scatter

* fix typo

* refactor from d3-format to superset-ui
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@villebro villebro villebro left review comments

@mistercrunch mistercrunch mistercrunch approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v0.34 🍒 0.34.1 🚢 0.35.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thunter009 @etr2460 @codecov-io @villebro @mistercrunch