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

[Lyft-GA] Enable color consistency in a dashboard #7135

Merged
merged 7 commits into from
Mar 29, 2019

Conversation

khtruong
Copy link
Contributor

Accidentally messed up rebasing and it seemed easier to just create a new branch.

Refer to this PR for all details: #7086

Moved actions, minor UI, allowed dashboard copy

Fix linting errors

Undo unintentional change

Updated and added unit tests

Fail quietly if package has not been updated

Fail quietly on dashboard copy if package is old
@codecov-io
Copy link

codecov-io commented Mar 27, 2019

Codecov Report

Merging #7135 into lyftga will decrease coverage by 0.1%.
The diff coverage is 27.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           lyftga    #7135      +/-   ##
==========================================
- Coverage   64.34%   64.23%   -0.11%     
==========================================
  Files         423      425       +2     
  Lines       20597    20672      +75     
  Branches     2255     2270      +15     
==========================================
+ Hits        13253    13279      +26     
- Misses       7211     7260      +49     
  Partials      133      133
Impacted Files Coverage Δ
...sets/src/dashboard/containers/DashboardBuilder.jsx 0% <ø> (ø) ⬆️
...ssets/src/dashboard/containers/DashboardHeader.jsx 100% <ø> (ø) ⬆️
superset/assets/src/dashboard/util/propShapes.jsx 100% <ø> (ø) ⬆️
...explore/components/controls/ColorSchemeControl.jsx 46.66% <0%> (ø) ⬆️
superset/assets/src/chart/ChartRenderer.jsx 7.04% <0%> (-0.11%) ⬇️
...t/assets/src/dashboard/reducers/getInitialState.js 0% <0%> (ø) ⬆️
...rset/assets/src/dashboard/components/SaveModal.jsx 43.9% <0%> (-2.26%) ⬇️
.../src/dashboard/components/BuilderComponentPane.jsx 28.57% <0%> (+2.25%) ⬆️
...et/assets/src/dashboard/actions/dashboardLayout.js 97.46% <100%> (+0.03%) ⬆️
superset/assets/src/dashboard/util/constants.js 100% <100%> (ø) ⬆️
... and 15 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 87fb2df...153fb49. Read the comment docs.

@mistercrunch mistercrunch added risk:many-files Change has too many files enhancement:request Enhancement request submitted by anyone from the community .dashboard labels Mar 27, 2019
export function setColorSchemeAndUnsavedChanges(colorScheme) {
return dispatch => {
dispatch(setColorScheme(colorScheme));
dispatch(setUnsavedChanges(true));

Choose a reason for hiding this comment

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

setUnsavedChanges (and hasUnsavedChanges state) is shared with dashboard's layout change. If you change color scheme, change dashboard layout, then undo a all dashboard layout change, the hasUnsavedChanges flag will be reset.
you probably have to add some logic to make sure this flag carries changes from both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a bool flag to track unsaved color changes. I don't love the approach but since we want the layout stuff to be refactored to allow non-layout changes to be undo-able, this was the least amount of changes until that refactoring gets done.

onMouseEnter={this.onMouseEnter}
onMouseLeave={this.onMouseLeave}
>
<ColorSchemeControl

Choose a reason for hiding this comment

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

can you add scheme name as tooltip when user mouse over color scheme choices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me get @dorq 's opinion. The color name schemes may not make sense to users based on what we call them today.

Copy link

Choose a reason for hiding this comment

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

@khtruong and I discussed this offline, and we agreed that even though the color scheme names are not very user-friendly (e.g. 'lyftColors', 'bnbColors', 'd3Category10', d3Category20'), there are situations in which it would be valuable to see them. So adding them as a tooltip (as opposed to putting the scheme names directly into the dropdown options) is a good compromise.

@khtruong will add this to the PR (thanks, Kim!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to make them as user-friendly as possible (i.e. lyftColors -> Lyft Colors, bnbColors -> Airbnb Colors, etc)

Choose a reason for hiding this comment

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

i don't really mean to use scheme name (like lyftcolors bnbcolors), any user friendly descriptive name will be good. so that when i want to apply same color scheme all over multiple dashboards, it will be easier for me to remember.

Copy link
Contributor

@kristw kristw Mar 28, 2019

Choose a reason for hiding this comment

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

ColorScheme datastructure has label field, which falls back to id by default.
I think the code in this PR should use the label instead of the keys which are id.
Then we can update the color schemes in @superset-ui to include human-friendly label.

p.s. ColorScheme also has description field.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

@khtruong khtruong Mar 28, 2019

Choose a reason for hiding this comment

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

You are quick! I just updated the color schemes in @superset-ui to have actual labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

awesome!

@kristw kristw added the review label Mar 28, 2019
@vylc vylc added the lyft Related to Lyft label Mar 28, 2019
@graceguo-supercat
Copy link

Please see my screenshot. Color scheme is carried from dashboard to explore view, but color is not consistent for each label:

j9UBYSSIyG

@khtruong
Copy link
Contributor Author

khtruong commented Mar 28, 2019

Yeah. Unfortunately, the chart and dashboard coloring are completely separate. I chatted with design and PM on this. The issue is that charts can be part of multiple dashboards so which dashboard should carry over to the chart? So the color mapping is reset when you explore the chart. I think it appeared the color scheme carried over but I would guess that coincidentally the dashboard and chart both have the same color scheme. We haven't thought of a great to get a consistent experience.

@graceguo-supercat
Copy link

graceguo-supercat commented Mar 28, 2019

Yeah. Unfortunately, the chart and dashboard coloring are completely separate. I chatted with design and PM on this. The issue is that charts can be part of multiple dashboards so which dashboard should carry over to the chart? So the color scheme is reset when you explore the chart. I think it appeared the color scheme carried over but I would guess that coincidentally the dashboard and chart both have the same color scheme. We haven't thought of a great to get a consistent experience.

it's not coincident, because you added colorScheme param into charts' formData, and Explore chart action will open the chart with formData which is updated from dashboard. It's different from open chart by id from explore view, Explore chart will carry additional filter parameters from dashboard too.

In my opinion, chart opened from dashboard should be consistent with its look/data inside the dashboard.

@@ -49,7 +49,7 @@
"dependencies": {
"@data-ui/sparkline": "^0.0.54",
"@superset-ui/chart": "^0.9.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check versions. master has newer version for these @superset-ui/xxx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a branch off of lyftga. I think lyftga is a little behind.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh....haha my bad

@khtruong
Copy link
Contributor Author

Yeah. Unfortunately, the chart and dashboard coloring are completely separate. I chatted with design and PM on this. The issue is that charts can be part of multiple dashboards so which dashboard should carry over to the chart? So the color scheme is reset when you explore the chart. I think it appeared the color scheme carried over but I would guess that coincidentally the dashboard and chart both have the same color scheme. We haven't thought of a great to get a consistent experience.

it's not coincident, because you added colorScheme param into charts' formData, and Explore chart action will open the chart with formData which is updated from dashboard. It's different from open chart by id from explore view, Explore chart will carry additional filter parameters from dashboard too.

In my opinion, chart opened from dashboard should be consistent with its look/data inside the dashboard.

Eek. You're right. I was thinking of when I changed the colors and then went to the chart and explored it from the chart list. I have updated it to use the chart form data so that when you explore it, it will use the color scheme associated with that chart.

Yeah, I think there are differing opinions of what should happen. I personally am unsure and think we need to rethink about it from a broader perspective that covers all, or at least most, cases. It does feel more consistent to have what the dashboard set but we still have a problem if the chart is part of multiple dashboards that have different color schemes. Should the color scheme just be temporary? Should we indicate to the user that this is not the chart's saved color scheme? There hasn't been a general consensus yet. We can definitely discuss more for future PRs if you want to brainstorm a better experience. But for now, I will reset it to the chart's color scheme until there's been a clearer consensus of that experience should be.

@graceguo-supercat
Copy link

graceguo-supercat commented Mar 28, 2019

It does feel more consistent to have what the dashboard set but we still have a problem if the chart is part of multiple dashboards that have different color schemes.

Think about this:
you have a chart Population show world population, and you insert it into multiple dashboards. in dashboard_1 you have dashboard level filter say country=US, and dashboard_2 you have filter say country=UK.

  • if you open chart by itself, no filter, the chart show population for all over the world.
  • The same chart should show US population in dashboard_1.
  • The same chart should show UK population in dashboard_2.
  • if you click Explore chart button from dashboard_1, you should see chart show same number as it in dashboard_1.
    My opinion is, color scheme, should behave same as this country parameter.

Chart for sure will be used in multiple dashboards. If we open explore view from Explore chart button of a dashboard, the chart should show same data, as well as same label color, it was in that specific dashboard.

@khtruong
Copy link
Contributor Author

It does feel more consistent to have what the dashboard set but we still have a problem if the chart is part of multiple dashboards that have different color schemes.

Think about this:
you have a chart Population show world population, and you insert it into multiple dashboards. in dashboard_1 you have dashboard level filter say country=US, and dashboard_2 you have filter say country=UK.

  • if you open chart by itself, no filter, the chart show population for all over the world.
  • The same chart should show US population in dashboard_1.
  • The same chart should show UK population in dashboard_2.
  • if you click Explore chart button from dashboard_1, you should see chart show same number as it in dashboard_1.
    My opinion is, color scheme, should behave same as this country parameter.

Chart for sure will be used in multiple dashboards. If we open explore view from Explore chart button of a dashboard, the chart should show same data, as well as same label color, it was in that specific dashboard.

Good point! Grace and I discussed offline. We are in agreement on how the ideal scenario should be. But I think that leads to the next point: we need color consistency in charts as well. I'd like to address these in separate PR's that build on top of each other where each PR gets us to a better experience (It's always nicer to review smaller PRs if we can help it. I know I appreciate that :) ). So I'd like to keep it how it is and open up a git issue that will address color consistency in charts that includes having consistency when you explore charts from the dashboard. How's that sound?

@graceguo-supercat
Copy link

good. We can take care dashboard => explore view label consistency in another PR. Thanks for the work!!

@khtruong
Copy link
Contributor Author

good. We can take care dashboard => explore view label consistency in another PR. Thanks for the work!!

Thanks for all the feedback!! You have been awesome.

@khtruong
Copy link
Contributor Author

good. We can take care dashboard => explore view label consistency in another PR. Thanks for the work!!

Issue has been created to track this work: #7163

@khtruong khtruong changed the title Enable color consistency in a dashboard [Lyft-GA] Enable color consistency in a dashboard Mar 28, 2019
@xtinec xtinec merged commit 71eea53 into apache:lyftga Mar 29, 2019
xtinec pushed a commit that referenced this pull request Mar 31, 2019
* Enable color consistency in a dashboard

Moved actions, minor UI, allowed dashboard copy

Fix linting errors

Undo unintentional change

Updated and added unit tests

Fail quietly if package has not been updated

Fail quietly on dashboard copy if package is old

* Update packages

* Remove unnecessary code

* Addressed Grace's comments

* Small fix for item key

* Reset chart's color during exploration

* Do not reset chart form data when exploring chart
xtinec pushed a commit that referenced this pull request Apr 2, 2019
* Sparkline dates aren't formatting in Time Series Table (#6976)

* Exclude venv for python linter to ignore

* Fix NaN error

* Fix the white background shown in SQL editor on drag (#7021)

This PR sets the background-color css property on `.ace_scroller` instead of `.ace_content` to prevent the white background shown during resizing of the SQL editor before drag ends.

* Show tooltip with time frame (#6979)

* Fix time filter control (#6978)

* Enhancement of query context and object. (#6962)

* added more functionalities for query context and object.

* fixed cache logic

* added default value for groupby

* updated comments and removed print

(cherry picked from commit d5b9795)

* [fix] /superset/slice/id url is too long (#6989)


(cherry picked from commit 6a4d507)

* [WIP] fix user specified JSON metadata not updating dashboard on refresh (#7027)


(cherry picked from commit cc58f0e)

* feat: add ability to change font size in big number (#7003)

* Add ability to change font sizes in Big Number

* rename big number to header

* Add comment to clarify font size values

* Allow LIMIT to be specified in parameters (#7052)

* [fix] Cursor jumping when editing chart and dashboard titles (#7038)


(cherry picked from commit fc1770f)

* Changing time table viz to pass formatTime a date (#7020)

(cherry picked from commit 7f3c145)

* [db-engine-spec] Aligning Hive/Presto partition logic (#7007)


(cherry picked from commit 05be866)

* [fix] explore chart from dashboard missed slice title (#7046)


(cherry picked from commit a6d48d4)

* fix inaccurate data calculation with adata rolling and contribution (#7035)


(cherry picked from commit 0782e83)

* Adding warning message for sqllab save query (#7028)


(cherry picked from commit ead3d48)

* [datasource] Ensuring consistent behavior of datasource editing/saving. (#7037)

* Update datasource.py

* Update datasource.py

(cherry picked from commit c771625)

* [csv-upload] Fixing message encoding (#6971)


(cherry picked from commit 48431ab)

* [sql-parse] Fixing LIMIT exceptions (#6963)


(cherry picked from commit 3e076cb)

* Adding custom control overrides (#6956)

* Adding extraOverrides to line chart

* Updating extraOverrides to fit with more cases

* Moving extraOverrides to index.js

* Removing webpack-merge in package.json

* Fixing metrics control clearing metric

(cherry picked from commit e619405)

* [sqlparse] Fixing table name extraction for ill-defined query (#7029)


(cherry picked from commit 07c340c)

* [missing values] Removing replacing missing values (#4905)


(cherry picked from commit 61add60)

* [SQL Lab] Improved query and results tabs rendering reliability (#7082)

closes #7080

(cherry picked from commit 9b58e9f)

* Fix filter_box migration PR #6523 (#7066)

* Fix filter_box migration PR #6523

* Fix druid-related bug

(cherry picked from commit b210742)

* SQL editor layout makeover (#7102)

This PR includes the following layout and css tweaks:
- Using flex to layout the north and south sub panes of query pane so resizing works properly in both Chrome and Firefox
- Removal of necessary wrapper divs and tweaking of css in sql lab so we can scroll to the bottom of both the table list and the results pane
- Make sql lab's content not overflow vertically and layout the query result area to eliminate double scroll bars
- css tweaks on the basic.html page so the loading animation appears in the center of the page across the board

(cherry picked from commit 71f1bbd)

* [forms] Fix handling of NULLs

(cherry picked from commit e83a07d)

* handle null column_name in sqla and druid models

(cherry picked from commit 2ff721a)

* Use metric name instead of metric in filter box (#7106)


(cherry picked from commit 003364e)

* Bump python lib croniter to an existing version (#7132)

Package maintainers should really never delete packages, but it appears
this happened with croniter and resulted in breaking our builds.

This PR bumps to a more recent existing version of the library

(cherry picked from commit 215ed39)

* Revert PR #6933 (#7162)

* [bugfix] SQL Lab 'Filter Results' doesn't stick (#7104)

When using a "Search Results" criteria, the subset of rows that match
the criteria get displayed. While this the filter is applied, if another
query is run, the filter is still active, but not displayed in the input
text box. After this change, the state of the input box sticks after
subsequent queries.

(cherry picked from commit d5e8d66)

* Injectable statsd client (#7138)

* Add ability to inject statsd client; some py test/reqs updates

- Updated the metrics logger to allow construction with an existing
statsd client, so that it can be configured by external systems or libs.
- added requirements to requirements-dev.txt which are needed to run
  tests-eg coverage, nose
- removed dependency on mock lib, it is in python stdlib now
- updated tox.ini to remove the now-superfluous deps

* add license to test file, and remove blank line at EOF

(cherry picked from commit ba19a62)

* [Lyft-GA] Enable color consistency in a dashboard (#7135)

* Enable color consistency in a dashboard

Moved actions, minor UI, allowed dashboard copy

Fix linting errors

Undo unintentional change

Updated and added unit tests

Fail quietly if package has not been updated

Fail quietly on dashboard copy if package is old

* Update packages

* Remove unnecessary code

* Addressed Grace's comments

* Small fix for item key

* Reset chart's color during exploration

* Do not reset chart form data when exploring chart

* Fix double scroll bars when content of sql result table overflows horizontally (#7168)

The PR substracts the scrollbar height from the height of the container of the react virtualized table so we don't see double scrollbars.

(cherry picked from commit 7ffcabd)

* Change number format default

* Use smart formatter instead

* fix merge issues

* Use SMART_NUMBER
xtinec pushed a commit that referenced this pull request Apr 2, 2019
* Enable color consistency in a dashboard

Moved actions, minor UI, allowed dashboard copy

Fix linting errors

Undo unintentional change

Updated and added unit tests

Fail quietly if package has not been updated

Fail quietly on dashboard copy if package is old

* Update packages

* Remove unnecessary code

* Addressed Grace's comments

* Small fix for item key

* Reset chart's color during exploration

* Do not reset chart form data when exploring chart
xtinec pushed a commit that referenced this pull request Apr 2, 2019
* Sparkline dates aren't formatting in Time Series Table (#6976)

* Exclude venv for python linter to ignore

* Fix NaN error

* Fix the white background shown in SQL editor on drag (#7021)

This PR sets the background-color css property on `.ace_scroller` instead of `.ace_content` to prevent the white background shown during resizing of the SQL editor before drag ends.

* Show tooltip with time frame (#6979)

* Fix time filter control (#6978)

* Enhancement of query context and object. (#6962)

* added more functionalities for query context and object.

* fixed cache logic

* added default value for groupby

* updated comments and removed print

(cherry picked from commit d5b9795)

* [fix] /superset/slice/id url is too long (#6989)


(cherry picked from commit 6a4d507)

* [WIP] fix user specified JSON metadata not updating dashboard on refresh (#7027)


(cherry picked from commit cc58f0e)

* feat: add ability to change font size in big number (#7003)

* Add ability to change font sizes in Big Number

* rename big number to header

* Add comment to clarify font size values

* Allow LIMIT to be specified in parameters (#7052)

* [fix] Cursor jumping when editing chart and dashboard titles (#7038)


(cherry picked from commit fc1770f)

* Changing time table viz to pass formatTime a date (#7020)

(cherry picked from commit 7f3c145)

* [db-engine-spec] Aligning Hive/Presto partition logic (#7007)


(cherry picked from commit 05be866)

* [fix] explore chart from dashboard missed slice title (#7046)


(cherry picked from commit a6d48d4)

* fix inaccurate data calculation with adata rolling and contribution (#7035)


(cherry picked from commit 0782e83)

* Adding warning message for sqllab save query (#7028)


(cherry picked from commit ead3d48)

* [datasource] Ensuring consistent behavior of datasource editing/saving. (#7037)

* Update datasource.py

* Update datasource.py

(cherry picked from commit c771625)

* [csv-upload] Fixing message encoding (#6971)


(cherry picked from commit 48431ab)

* [sql-parse] Fixing LIMIT exceptions (#6963)


(cherry picked from commit 3e076cb)

* Adding custom control overrides (#6956)

* Adding extraOverrides to line chart

* Updating extraOverrides to fit with more cases

* Moving extraOverrides to index.js

* Removing webpack-merge in package.json

* Fixing metrics control clearing metric

(cherry picked from commit e619405)

* [sqlparse] Fixing table name extraction for ill-defined query (#7029)


(cherry picked from commit 07c340c)

* [missing values] Removing replacing missing values (#4905)


(cherry picked from commit 61add60)

* [SQL Lab] Improved query and results tabs rendering reliability (#7082)

closes #7080

(cherry picked from commit 9b58e9f)

* Fix filter_box migration PR #6523 (#7066)

* Fix filter_box migration PR #6523

* Fix druid-related bug

(cherry picked from commit b210742)

* SQL editor layout makeover (#7102)

This PR includes the following layout and css tweaks:
- Using flex to layout the north and south sub panes of query pane so resizing works properly in both Chrome and Firefox
- Removal of necessary wrapper divs and tweaking of css in sql lab so we can scroll to the bottom of both the table list and the results pane
- Make sql lab's content not overflow vertically and layout the query result area to eliminate double scroll bars
- css tweaks on the basic.html page so the loading animation appears in the center of the page across the board

(cherry picked from commit 71f1bbd)

* [forms] Fix handling of NULLs

(cherry picked from commit e83a07d)

* handle null column_name in sqla and druid models

(cherry picked from commit 2ff721a)

* Use metric name instead of metric in filter box (#7106)


(cherry picked from commit 003364e)

* Bump python lib croniter to an existing version (#7132)

Package maintainers should really never delete packages, but it appears
this happened with croniter and resulted in breaking our builds.

This PR bumps to a more recent existing version of the library

(cherry picked from commit 215ed39)

* Revert PR #6933 (#7162)

* [bugfix] SQL Lab 'Filter Results' doesn't stick (#7104)

When using a "Search Results" criteria, the subset of rows that match
the criteria get displayed. While this the filter is applied, if another
query is run, the filter is still active, but not displayed in the input
text box. After this change, the state of the input box sticks after
subsequent queries.

(cherry picked from commit d5e8d66)

* Injectable statsd client (#7138)

* Add ability to inject statsd client; some py test/reqs updates

- Updated the metrics logger to allow construction with an existing
statsd client, so that it can be configured by external systems or libs.
- added requirements to requirements-dev.txt which are needed to run
  tests-eg coverage, nose
- removed dependency on mock lib, it is in python stdlib now
- updated tox.ini to remove the now-superfluous deps

* add license to test file, and remove blank line at EOF

(cherry picked from commit ba19a62)

* [Lyft-GA] Enable color consistency in a dashboard (#7135)

* Enable color consistency in a dashboard

Moved actions, minor UI, allowed dashboard copy

Fix linting errors

Undo unintentional change

Updated and added unit tests

Fail quietly if package has not been updated

Fail quietly on dashboard copy if package is old

* Update packages

* Remove unnecessary code

* Addressed Grace's comments

* Small fix for item key

* Reset chart's color during exploration

* Do not reset chart form data when exploring chart

* Fix double scroll bars when content of sql result table overflows horizontally (#7168)

The PR substracts the scrollbar height from the height of the container of the react virtualized table so we don't see double scrollbars.

(cherry picked from commit 7ffcabd)

* Change number format default

* Use smart formatter instead

* fix merge issues

* Use SMART_NUMBER
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 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 enhancement:request Enhancement request submitted by anyone from the community lyft Related to Lyft risk:many-files Change has too many files 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants