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

fix(Dashboard): Color inconsistency on refreshes and conflicts #27439

Merged
merged 21 commits into from
Jun 20, 2024
Merged

fix(Dashboard): Color inconsistency on refreshes and conflicts #27439

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
91f0d51
Merge branch 'master' of https://github.com/apache/superset into dieg…
geido Mar 1, 2024
43fe162
Prefer less used colors
geido Mar 1, 2024
51bf74f
Merge branch 'master' of https://github.com/apache/superset into dieg…
geido Mar 1, 2024
6f89a9c
Update map on hidden tabs
geido Mar 8, 2024
83cf930
Clean up
geido Mar 8, 2024
a3e7ae8
Apply same color to same label value
geido Mar 8, 2024
7706426
Update tests
geido Mar 8, 2024
664d799
Update e2e tests
geido Mar 8, 2024
36795f2
Increase coverage
geido Mar 8, 2024
f7dc117
Merge branch 'master' of https://github.com/apache/superset into dieg…
geido Apr 30, 2024
fa0e830
Refactor: naming - centralize update [WIP]
geido May 2, 2024
211bfc0
Merge branch 'master' of https://github.com/apache/superset into dieg…
geido May 7, 2024
eadc4c3
[WIP] Refactor saving logics
geido May 9, 2024
2eed5e5
Merge
geido May 13, 2024
c81a55f
Clean up
geido May 13, 2024
33e0c1d
Move utils outside dashboard
geido May 14, 2024
12a9a39
Re-apply chart original scheme
geido May 15, 2024
8bdbb2e
Fix coverage
geido May 15, 2024
98e620b
Merge branch 'master' of https://github.com/apache/superset into dieg…
geido May 28, 2024
3e6c6a5
Merge branch 'master' of https://github.com/apache/superset into dieg…
geido May 30, 2024
636e9bd
Merge
geido Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions RESOURCES/FEATURE_FLAGS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ These features flags currently default to True and **will be removed in a future

[//]: # "PLEASE KEEP THE LIST SORTED ALPHABETICALLY"

- AVOID_COLORS_COLLISION
Copy link
Member

Choose a reason for hiding this comment

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

We might technically need to get consensus on this. @michael-s-molina any problems getting consensus and doing this now, or should we wait until the 5.0 proposals open up?

Copy link
Member

Choose a reason for hiding this comment

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

I think waiting for 5.0 would be a good idea as it allows an orderly evaluation of proposed deprecations. We just need to add a card to the board to remind us to submit the proposal.

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-s-molina @rusackas this feature flag introduced a regression, so I don't think it makes sense to discuss depreciation. I am keeping it here and not deleting it for now just because we don't have a breaking window open. Thoughts?

- DASHBOARD_CROSS_FILTERS
- ENABLE_JAVASCRIPT_CONTROLS
- KV_STORE
49 changes: 20 additions & 29 deletions superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ function selectColorScheme(color: string) {
)
.first()
.click();
cy.getBySel(color).click();
cy.getBySel(color).click({ force: true });
}

function applyChanges() {
Expand Down Expand Up @@ -169,6 +169,7 @@ function writeMetadata(metadata: string) {

function openExplore(chartName: string) {
interceptExploreJson();
interceptGet();

cy.get(
`[data-test-chart-name='${chartName}'] [aria-label='More Options']`,
Expand Down Expand Up @@ -210,7 +211,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(69, 78, 124)');
.should('have.css', 'fill', 'rgb(31, 168, 201)');
geido marked this conversation as resolved.
Show resolved Hide resolved
});

it('should apply same color to same labels with color scheme set', () => {
Expand All @@ -231,7 +232,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(51, 61, 71)');
.should('have.css', 'fill', 'rgb(234, 11, 140)');

// open 2nd main tab
openTab(0, 1);
Expand All @@ -240,7 +241,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
.should('have.css', 'fill', 'rgb(51, 61, 71)');
.should('have.css', 'fill', 'rgb(234, 11, 140)');
});

it('should apply same color to same labels with no color scheme set', () => {
Expand All @@ -261,7 +262,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(69, 78, 124)');
.should('have.css', 'fill', 'rgb(31, 168, 201)');

// open 2nd main tab
openTab(0, 1);
Expand All @@ -270,7 +271,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
.should('have.css', 'fill', 'rgb(69, 78, 124)');
.should('have.css', 'fill', 'rgb(31, 168, 201)');
});

it('custom label colors should take the precedence in nested tabs', () => {
Expand Down Expand Up @@ -384,17 +385,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(69, 78, 124)');
.should('have.css', 'fill', 'rgb(31, 168, 201)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
.should('have.css', 'fill', 'rgb(224, 67, 85)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
.should('have.css', 'fill', 'rgb(163, 143, 121)');
.should('have.css', 'fill', 'rgb(90, 193, 137)');

openProperties();
cy.get('[aria-label="Select color scheme"]').should('have.value', '');
Expand Down Expand Up @@ -423,17 +424,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(69, 78, 124)');
.should('have.css', 'fill', 'rgb(31, 168, 201)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
.should('have.css', 'fill', 'rgb(224, 67, 85)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
.should('have.css', 'fill', 'rgb(163, 143, 121)');
.should('have.css', 'fill', 'rgb(90, 193, 137)');
});

it('should show the same colors in Explore', () => {
Expand All @@ -459,23 +460,13 @@ describe('Dashboard edit', () => {
)
.first()
.should('have.css', 'fill', 'rgb(255, 0, 0)');
// label Christopher
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
.should('have.css', 'fill', 'rgb(172, 32, 119)');

openExplore('Top 10 California Names Timeseries');

// label Anthony
cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(255, 0, 0)');
// label Christopher
cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
.eq(1)
.should('have.css', 'fill', 'rgb(172, 32, 119)');
});

it('should change color scheme multiple times', () => {
Expand All @@ -496,7 +487,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(51, 61, 71)');
.should('have.css', 'fill', 'rgb(234, 11, 140)');

// open 2nd main tab
openTab(0, 1);
Expand All @@ -505,7 +496,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
.should('have.css', 'fill', 'rgb(51, 61, 71)');
.should('have.css', 'fill', 'rgb(234, 11, 140)');

editDashboard();
openProperties();
Expand All @@ -516,7 +507,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
.should('have.css', 'fill', 'rgb(244, 176, 42)');
.should('have.css', 'fill', 'rgb(41, 105, 107)');

// open main tab and nested tab
openTab(0, 0);
Expand All @@ -527,7 +518,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(244, 176, 42)');
.should('have.css', 'fill', 'rgb(41, 105, 107)');
});

it('should apply the color scheme across main tabs', () => {
Expand All @@ -542,7 +533,7 @@ describe('Dashboard edit', () => {

cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(51, 61, 71)');
.should('have.css', 'fill', 'rgb(234, 11, 140)');
});

it('should apply the color scheme across main tabs for rendered charts', () => {
Expand All @@ -558,7 +549,7 @@ describe('Dashboard edit', () => {

cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(156, 52, 152)');
.should('have.css', 'fill', 'rgb(41, 105, 107)');

// change scheme now that charts are rendered across the main tabs
editDashboard();
Expand Down Expand Up @@ -588,7 +579,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
.should('have.css', 'fill', 'rgb(51, 61, 71)');
.should('have.css', 'fill', 'rgb(234, 11, 140)');

// open another nested tab
openTab(2, 1);
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/cypress-base/cypress/e2e/explore/visualizations/compare.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,6 @@ describe('Visualization > Compare', () => {
).should('exist');
cy.get('.compare .nv-legend .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(31, 168, 201)');
.should('have.css', 'fill', 'rgb(69, 78, 124)');
});
});
2 changes: 1 addition & 1 deletion superset-frontend/cypress-base/cypress/e2e/explore/visualizations/dist_bar.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,6 @@ describe('Visualization > Distribution bar chart', () => {
).should('exist');
cy.get('.dist_bar .nv-legend .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(41, 105, 107)');
.should('have.css', 'fill', 'rgb(91, 202, 206)');
});
});
Loading
Loading