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

[SIP-5] Refactor and repair partition #5718

Merged
merged 7 commits into from
Aug 29, 2018
Merged

[SIP-5] Refactor and repair partition #5718

merged 7 commits into from
Aug 29, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Aug 24, 2018 •
edited
Loading

  • Extract slice and formData according to [SIP-5]
  • Fix small issues and syntax inside heatmap.
  • Fix broken tooltip: Add background, reorder columns into readable format. add padding between columns

@williaster @conglei

Before
icicle_test-2

After
icicle_test

@kristw kristw mentioned this pull request Aug 24, 2018
Closed
@kristw kristw changed the title Refactor and repair partition Refactor and repair partition [SIP-5] Aug 24, 2018
@kristw kristw changed the title Refactor and repair partition [SIP-5] [SIP-5] Refactor and repair partition Aug 24, 2018
@codecov-io
Copy link

codecov-io commented Aug 24, 2018 •
edited
Loading

Codecov Report

Merging #5718 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5718      +/-   ##
==========================================
- Coverage   63.43%   63.38%   -0.05%     
==========================================
  Files         361      361              
  Lines       22977    22995      +18     
  Branches     2558     2560       +2     
==========================================
  Hits        14575    14575              
- Misses       8387     8405      +18     
  Partials       15       15
Impacted Files Coverage Δ
superset/assets/src/visualizations/partition.js 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 2abc941...40c5d4b. Read the comment docs.

williaster
williaster approved these changes Aug 29, 2018
View reviewed changes
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

couple comments, looks solid overall! 🎈

superset/assets/src/visualizations/partition.css
fill: rgba(0, 0, 0, 0.8);
}

.partition g:hover text {
fill: rgba(0, 0, 0, 1);
}

.partition .partition-tooltip {
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice at some point to have consistent colors/padding across things like chart tooltips. not sure how to best do that in the plugin world... css in javascript makes it easy to pass variables around. something to think about.

Copy link
Contributor Author

@kristw kristw Aug 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I would love to get rid of in-chart tooltips and make all use vx tooltip. Can do another sweep after converting these to react

superset/assets/src/visualizations/partition.js
import './partition.css';

d3.hierarchy = require('d3-hierarchy').hierarchy;
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting 🤔

superset/assets/src/visualizations/partition.js
.append('div')
.attr('class', 'nvtooltip')
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

superset/assets/src/visualizations/partition.js Outdated
return d.color;
});

// Zoom out when clicking outside vis
Copy link
Contributor

Choose a reason for hiding this comment

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

should we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm. will do.

superset/assets/src/visualizations/partition.js Outdated

const nodes = init(root);

let kx = w / root.dx;
Copy link
Contributor

Choose a reason for hiding this comment

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

these var names are a little cryptic but nbd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must be from the blocks.org examples

superset/assets/src/visualizations/partition.js Outdated
logScale,
metrics,
numberFormat,
partitionLimit: partitionLimit ? parseInt(partitionLimit, 10) : partitionLimit,
Copy link
Contributor

Choose a reason for hiding this comment

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

have seen this flagged by linting (outside of Superset) too many times to not comment 😬 these two could be simplified to partitionLimit && parseInt(partitionLimit, 10)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

superset/assets/src/visualizations/partition.js Outdated
);
t += (
'<tr class="emph">' +
'<td class="legend-color-guide" style="opacity: 0.75">' +
Copy link
Contributor

Choose a reason for hiding this comment

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

do styles for .legend-color-guide and .x-value come from another style sheet? or do they do anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Umm, this might be an nvd3 thing. I did not touch this part.

@kristw
Copy link
Contributor Author

kristw commented Aug 29, 2018 •
edited
Loading

I addressed the comments above.

  • Remove commented code
  • Rename kx ky to zoomX and zoomY, respectively.
  • Remove legacy className from nvd3 tooltip
  • Remove verboseMap from prop.

@codecov-io
Copy link

codecov-io commented Aug 29, 2018 •
edited
Loading

Codecov Report

Merging #5718 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5718      +/-   ##
==========================================
- Coverage    63.8%   63.75%   -0.05%     
==========================================
  Files         364      364              
  Lines       23066    23083      +17     
  Branches     2568     2568              
==========================================
  Hits        14717    14717              
- Misses       8334     8351      +17     
  Partials       15       15
Impacted Files Coverage Δ
superset/assets/src/visualizations/partition.js 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 00cc6e9...a71c0d8. Read the comment docs.

@williaster williaster merged commit 00f2771 into apache:master Aug 29, 2018
@kristw kristw deleted the kristw-partition branch August 29, 2018 22:25
kristw added a commit to kristw/incubator-superset that referenced this pull request Sep 5, 2018
@kristw
[SIP-5] Refactor and repair partition (apache#5718)
32423e6 
* Extract slice and formData

* reorder functions

* fix tooltip

* remove commented code

* remove commented code and rename variables

* remove verboseMap

* rename kx, ky to zoomX, zoomY

(cherry picked from commit 00f2771)
@kristw kristw mentioned this pull request Sep 5, 2018
Merged
@conglei conglei mentioned this pull request Sep 11, 2018
Closed
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
@kristw @betodealmeida
[SIP-5] Refactor and repair partition (apache#5718)
58feb2e 
* Extract slice and formData

* reorder functions

* fix tooltip

* remove commented code

* remove commented code and rename variables

* remove verboseMap

* rename kx, ky to zoomX, zoomY
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@kristw @williaster
[SIP-5] Refactor and repair partition (apache#5718)
e93affa 
* Extract slice and formData

* reorder functions

* fix tooltip

* remove commented code

* remove commented code and rename variables

* remove verboseMap

* rename kx, ky to zoomX, zoomY
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williaster williaster williaster 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 🚢 0.28.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kristw @codecov-io @williaster @mistercrunch