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

Separate vis-specific controls from centralized controls #6827

Closed
wants to merge 5 commits into from

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Feb 7, 2019

Summary

Allow each visualization to define its own controls, instead of storing all controls in centralized controls.jsx

image

Test plan

Verified that the rose and paired t-test charts that are modified in this PR are working correctly.

image

image

@mistercrunch @williaster @michellethomas @graceguo-supercat @conglei @khtruong @xtinec

@kristw kristw force-pushed the kristw--controls branch from c3ef95f to 599e462 Compare March 18, 2019 19:36
@kristw kristw force-pushed the kristw--controls branch from 599e462 to 81a5845 Compare April 18, 2019 22:47
@kristw kristw changed the title [WIP] Separate vis-specific controls from centralized controls Separate vis-specific controls from centralized controls Apr 18, 2019
@codecov-io
Copy link

codecov-io commented Apr 18, 2019

Codecov Report

Merging #6827 into master will decrease coverage by 0.03%.
The diff coverage is 64.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6827      +/-   ##
==========================================
- Coverage   64.94%   64.91%   -0.04%     
==========================================
  Files         424      424              
  Lines       20598    20613      +15     
  Branches     2281     2292      +11     
==========================================
+ Hits        13378    13381       +3     
- Misses       7097     7110      +13     
+ Partials      123      122       -1
Impacted Files Coverage Δ
superset/assets/src/explore/controls.jsx 42.4% <ø> (ø) ⬆️
superset/assets/src/explore/store.js 61.64% <43.33%> (-0.2%) ⬇️
.../src/explore/components/ControlPanelsContainer.jsx 67.94% <82.35%> (-12.06%) ⬇️

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 07d9dbd...4fe63c0. Read the comment docs.

@kristw kristw force-pushed the kristw--controls branch from 4fe63c0 to 05978c2 Compare April 19, 2019 17:19
@kristw
Copy link
Contributor Author

kristw commented Apr 24, 2019

@mistercrunch Saw that you are doing a heavy refactor on the control panel so I am not sure when should I land this one?

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, can see what Max says tho?

@kristw
Copy link
Contributor Author

kristw commented Apr 25, 2019

related to #7350
gonna have huge conflict.

@mistercrunch
Copy link
Member

mistercrunch commented Apr 30, 2019

@kristw on PR #7350 I'm mostly just touching/refactoring store.js, and I think most of your changes on that file here are around naming (except for getControlItems where you go a bit deeper...). I could add flow + flatMap on my side.

Maybe you can leave your changes out on that file so I can get my PR in? Otherwise it looks like it will conflict pretty hard.

My PR addresses some of the bad names too and breaks down the big functions into smaller chunks and introduces decent test coverage.

@stale
Copy link

stale bot commented Jun 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Jun 29, 2019
@stale stale bot closed this Jul 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants