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

[SIP-6] Migrate visualizations to new directory structure (part 2) #5997

Merged
merged 7 commits into from Oct 4, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Sep 27, 2018

Included in this PR
BigNumber
MapBox
TimeTable
EventFlow

Remaining for next PR
nvd3
iframe
markup
deck.gl

@conglei @williaster

@codecov-io
Copy link

codecov-io commented Sep 29, 2018

Codecov Report

Merging #5997 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5997      +/-   ##
==========================================
- Coverage   64.66%   64.65%   -0.02%     
==========================================
  Files         448      454       +6     
  Lines       23773    23793      +20     
  Branches     2639     2641       +2     
==========================================
+ Hits        15373    15383      +10     
- Misses       8387     8397      +10     
  Partials       13       13
Impacted Files Coverage Δ
...perset/assets/src/visualizations/MapBox/MapBox.jsx 0% <0%> (ø) ⬆️
.../assets/src/visualizations/EventFlow/EventFlow.jsx 0% <0%> (ø)
.../assets/src/visualizations/TimeTable/TimeTable.jsx 0% <0%> (ø) ⬆️
...et/assets/src/visualizations/EventFlow/adaptor.jsx 0% <0%> (ø)
.../assets/src/visualizations/BigNumber/BigNumber.jsx 0% <0%> (ø) ⬆️
...ets/src/visualizations/TimeTable/transformProps.js 0% <0%> (ø)
...ets/src/visualizations/BigNumber/transformProps.js 0% <0%> (ø)
...ets/src/visualizations/EventFlow/transformProps.js 0% <0%> (ø)
...et/assets/src/visualizations/BigNumber/adaptor.jsx 0% <0%> (ø) ⬆️
...erset/assets/src/visualizations/MapBox/adaptor.jsx 0% <0%> (ø)
... and 12 more

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 a9ef0ae...46ff2c5. Read the comment docs.

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.

LGTM with the one thought on ids

min: Infinity,
max: -Infinity,
});
opts.map = prop => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

wow this is an interesting api

bigNumber,
className,
formatBigNumber: formatValue,
gradientId: `big_number_${containerId}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

in a world where everything is react, we won't have ids like this right? I wonder if instead of exposing the container id and creating a gradient id based on it, whether we can just create a unique id using the shortid package we use elsewhere. I guess ideally it doesn't change every render 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting idea. I like it. Can create the id in the constructor so it will not change every render.

@kristw kristw force-pushed the kristw-vis-dir2 branch 2 times, most recently from fa165b0 to d7c5a40 Compare October 3, 2018 19:43
@williaster williaster merged commit 9f028cc into apache:master Oct 4, 2018
@kristw kristw deleted the kristw-vis-dir2 branch October 4, 2018 21:47
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
…pache#5997)

* migrate MapBox

* migrate bignumber

* migrate timeseries table

* migrate EventFlow

* add default null

* fix linting

* use shortid instead of passing containerId
@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
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants