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

Add additional heatmap schemas #5549

Merged
merged 25 commits into from
Aug 14, 2018

Conversation

jerowe
Copy link
Contributor

@jerowe jerowe commented Aug 2, 2018

References: #4574
ping @mistercrunch

@jerowe jerowe changed the title Add additional heatmap schemas [WIP] - Add additional heatmap schemas Aug 2, 2018
@jerowe
Copy link
Contributor Author

jerowe commented Aug 2, 2018

I'm not done, do not merge!

@mistercrunch
Copy link
Member

Mind sharing screenshots of what the new options look like?

@jerowe
Copy link
Contributor Author

jerowe commented Aug 3, 2018

In theory I could, but somehow I broke the npm build and I'm trying to get it working again. Once I get it working I am happy to post screenshots.

@codecov-io
Copy link

codecov-io commented Aug 3, 2018

Codecov Report

Merging #5549 into master will increase coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5549      +/-   ##
==========================================
+ Coverage   63.36%   63.59%   +0.23%     
==========================================
  Files         351      359       +8     
  Lines       22258    22802     +544     
  Branches     2470     2534      +64     
==========================================
+ Hits        14104    14502     +398     
- Misses       8139     8285     +146     
  Partials       15       15
Impacted Files Coverage Δ
superset/assets/src/explore/controls.jsx 47.36% <ø> (+0.39%) ⬆️
superset/assets/src/modules/colors.js 77.55% <ø> (ø) ⬆️
...explore/components/controls/SelectAsyncControl.jsx 56% <0%> (-14%) ⬇️
...erset/assets/src/explore/actions/exploreActions.js 56.94% <0%> (-12.29%) ⬇️
superset/assets/src/components/Button.jsx 90.9% <0%> (-9.1%) ⬇️
...ets/src/SqlLab/components/ExploreResultsButton.jsx 82.97% <0%> (-4.21%) ⬇️
...rc/explore/components/controls/CheckboxControl.jsx 89.47% <0%> (-3.86%) ⬇️
.../explore/components/controls/DatasourceControl.jsx 66.17% <0%> (-1.83%) ⬇️
superset/models/core.py 85.16% <0%> (-1.41%) ⬇️
superset/views/core.py 73.83% <0%> (-0.15%) ⬇️
... and 23 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 b7f9dab...4046661. Read the comment docs.

@jerowe
Copy link
Contributor Author

jerowe commented Aug 6, 2018

Pinging @mistercrunch

screenshot 2018-08-06 12 16 31

screenshot 2018-08-06 12 15 44

screenshot 2018-08-06 12 15 26

screenshot 2018-08-06 12 11 19

@jerowe
Copy link
Contributor Author

jerowe commented Aug 6, 2018

@mistercrunch - I was thinking about changing some of the d3 colorschemes so that the first color (which is close to white) is dropped, but it seems that now missing blocks are gray. So maybe its not such a big deal?

@jerowe
Copy link
Contributor Author

jerowe commented Aug 6, 2018

FYI - I lifted all the color schemas straight from here - https://github.com/d3/d3-scale-chromatic

@jerowe
Copy link
Contributor Author

jerowe commented Aug 6, 2018

Hmmm I'm getting an error when I navigate to a dashboard, but I don't know why.

http://localhost:8088/superset/dashboard/deck/
http://localhost:8088/superset/dashboard/deck/

Uncaught TypeError: Cannot read property 'children' of undefined
    at t.default (dashboard.b259de2c9caaee2d7265.entry.js:111)
    at t.default (dashboard.b259de2c9caaee2d7265.entry.js:111)
    at Object.<anonymous> (dashboard.b259de2c9caaee2d7265.entry.js:102)
    at s (dashboard.b259de2c9caaee2d7265.entry.js:1)
    at Object.<anonymous> (dashboard.b259de2c9caaee2d7265.entry.js:102)
    at s (dashboard.b259de2c9caaee2d7265.entry.js:1)
    at n (dashboard.b259de2c9caaee2d7265.entry.js:1)
    at dashboard.b259de2c9caaee2d7265.entry.js:1
    at dashboard.b259de2c9caaee2d7265.entry.js:1

@mistercrunch
Copy link
Member

@jerowe there was a bug on master, rebase your branch should fix that, also please remove the [WiP] from the PR title if ready for review

@jerowe
Copy link
Contributor Author

jerowe commented Aug 6, 2018

I will wait for the tests to complete and then remove the WIP.

@jerowe jerowe changed the title [WIP] - Add additional heatmap schemas Add additional heatmap schemas Aug 6, 2018
@jerowe
Copy link
Contributor Author

jerowe commented Aug 6, 2018

@mistercrunch this is all ready

@jerowe
Copy link
Contributor Author

jerowe commented Aug 6, 2018

@mistercrunch , you see how in the calendar view the missing ones are grayed out? I like this. How possible would it be to get a PR to have that as an option for the other heatmap?

@jerowe
Copy link
Contributor Author

jerowe commented Aug 7, 2018

ping @mistercrunch (sorry to be a pain but I really want this.)

@jerowe
Copy link
Contributor Author

jerowe commented Aug 7, 2018

Ok and as long as we are talking about stuff I want:

Color select for missing values
Color select for values that don't make some cutoff (I think this will be harder since I always do the cutoff through sql )

@mistercrunch
Copy link
Member

Indents seem off, should be 2 not 4. I don't understand why the linter doesn't fire on that.

@jerowe
Copy link
Contributor Author

jerowe commented Aug 7, 2018

I tried to get my editor to go along with the project configs, but didn't quite get it perfect. Any tips? I use webstorm

@mistercrunch
Copy link
Member

Use vim :)

@jerowe
Copy link
Contributor Author

jerowe commented Aug 9, 2018

@mistercrunch , I fixed the indentation issues.

@mistercrunch
Copy link
Member

@elibrumbaugh @williaster what do you think of adding these color schemes?

@jerowe
Copy link
Contributor Author

jerowe commented Aug 9, 2018

ok now it is all ready

@@ -324,6 +324,40 @@ export const controls = {
['red_yellow_blue', 'red/yellowish/blue'],
['brown_white_green', 'brown/white/green'],
['purple_white_green', 'purple/white/green'],
['schemeBrBG', 'd3BrBG'],
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please make these more human readable? non-power users are not going to have any idea what BrBG means

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. updated.

@williaster
Copy link
Contributor

williaster commented Aug 9, 2018

I'm fine with it, this isn't the long term solution we want (ideally Superset admins would be able to define the color schemes they want in a plugin-way) but fine for now 🤷‍♀️

cc @kristw

@kristw
Copy link
Contributor

kristw commented Aug 9, 2018

LGTM. I agree with @williaster
@jerowe I would recommend squashing your git commits and clean the commit messages a little bit as these messages will forever be stored in the main repo git log.
https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

@jerowe
Copy link
Contributor Author

jerowe commented Aug 9, 2018

@kristw , I just tried to squash my commits, but I'm not sure I was successful. Can they get squashed on merge?

@mistercrunch mistercrunch merged commit a39dfb9 into apache:master Aug 14, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* got skeleton started

* added d3-scale-chromatic to package.json

* got hex values instead of calling from a function

* got rid of d3-scale-chromatic - no longer needed

* added schemas to controls

* damn editor broken some line spacing

* commit

* fix style issues

* whyyyyy won't this build

* whyyyyy won't this build

* damn typo

* hahaha got editor to deal with style configs

* no i guess i didn't

* gotta get them all

* again

* trying to get docker build ot work

* updated installation docs with some osx instructions

* restoring yarn.lock not sure why it changed

* trying to fix indent

* trying again

* CODE STYLE CHANGES WORK

* removing some colors that are too close to white

* human readable labels for names

* human readable labels for names
@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.

5 participants