chore: make control panel sub sections look better #24736

mistercrunch · 2023-07-18T21:59:01Z

SUMMARY

Improving the control panel section to make the hierarchy around the subsection look more clear. Currently it's unclear what is a section/sub-section/header. Bolding and adding an <hr/> as suggested by @kasiazjc helps quite a bit

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

after

rusackas · 2023-07-18T23:57:12Z

superset-frontend/packages/superset-ui-chart-controls/src/sections/advancedAnalytics.tsx

@@ -18,6 +18,8 @@
 */
 import React from 'react';
 import { t, RollingType, ComparisonType } from '@superset-ui/core';
+


The linter will likely whine about this extra line break, I would think.

it didn't, in python I like to add line breaks in-between external packages and local imports

rusackas

LGTM!

michael-s-molina · 2023-07-19T16:46:50Z

@kasiazjc @mistercrunch Could we try something that does not include the <hr>? For me, these additional lines kind of conflict with the collapsible panel lines. Maybe keep the bold and improve spacing?

kasiazjc · 2023-07-19T18:04:06Z

@kasiazjc @mistercrunch Could we try something that does not include the <hr>? For me, these additional lines kind of conflict with the collapsible panel lines. Maybe keep the bold and improve spacing?

@michael-s-molina I know that it's not perfect, but the line is a temp solution until we get rid of the upper case in labels (and in general in the app). With the current implementation and the number of different components and font sizes it's not easy to define a clear and easy hierarchy (we have to work with 12px font size too, because 14 is too big and overpowers the whole control panel). We had a version with line and without a line, and it seems for now the line makes the most sense (from the feedback I got), but curious to hear your opinion. Side by side mocks below:

note - there is one thing that needs to be changed in the current implementation - the color of the text in the subheader to #666666, because black is overpowering

What is the perfect case scenario is this version - labels are sentence case and there is no line below subheader, but it's sadly not something that we can take up right now

michael-s-molina · 2023-07-19T18:39:07Z

Thank you for the context @kasiazjc. I think that because of the uppercase limitation, neither solution is able to clearly define the information hierarchy at this point. I personally prefer the version without additional lines because it has less visual overload and it's more clear which items will be collapsed when I click on the collapse panel arrows. It's not a blocker for me though given that the solution to this problem lies in SIP-82.

john-bodley · 2023-07-19T19:10:22Z

+1 on @michael-s-molina's #24736 (comment). I don't disagree that the UX could be improved (see SIP-82 for details) but I'm not sure the proposal outlined in this PR actually makes things clearer. Actuality I think it makes things more convoluted (due to the additional of more lines).

Granted there's a margin around the horizontal lines, but I could perceive users being confused around the chevron behavior, i.e., for

it's not overly apparent (without really focusing on the elements) what will be collapsed when one would close the Advanced analytics section.

kasiazjc · 2023-07-19T20:27:45Z

+1 on @michael-s-molina's #24736 (comment). I don't disagree that the UX could be improved (see SIP-82 for details) but I'm not sure the proposal outlined in this PR actually makes things clearer. Actuality I think it makes things more convoluted (due to the additional of more lines).

Granted there's a margin around the horizontal lines, but I could perceive users being confused around the chevron behavior, i.e., for
it's not overly apparent (without really focusing on the elements) what will be collapsed when one would close the `Advanced analytics` section.

Thanks for feedback @john-bodley!

I hear you and @michael-s-molina and can see how the line could be confusing + it's cluttering control panel more. To be honest, internally, I've heard feedback both ways - as the hierarchy and typography is not perfect, in the end settled on this solution to try to make the subheader stand out more.

Saying that, from my perspective - I think in this iteration we can get rid of the line and keep the font weight, color + spacings, as this already is a big improvement. Hopefully we will get to the next phase soon and get rid of the upper case labels, so that the control panel will be less cluttered

mistercrunch · 2023-07-19T22:01:00Z

ok, killing the <hr/> for now. If I can get this merge I'd like to follow with a de-uppercasing form labels.

mistercrunch · 2023-07-19T22:09:07Z

Looks pretty clean to me just like that ->

mistercrunch · 2023-07-19T22:42:30Z

building on this PR is this one -> preset-io#583

Recent design proposals point towards removing the uppercase usage across the board, and this PR starts the process with form labels. it builds upon apache#24736 that started to address hierarchy-is-unclear issue in Explore control panel and makes things a bit neater on top of it.

codecov · 2023-07-19T23:32:23Z

Codecov Report

Merging #24736 (7beb901) into master (7675e0d) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

❗ Current head 7beb901 differs from pull request most recent head 494b9e8. Consider uploading reports for the commit 494b9e8 to get more accurate results

@@            Coverage Diff             @@
##           master   #24736      +/-   ##
==========================================
- Coverage   68.94%   68.94%   -0.01%     
==========================================
  Files        1901     1902       +1     
  Lines       73898    73902       +4     
  Branches     8175     8175              
==========================================
+ Hits        50951    50952       +1     
- Misses      20834    20837       +3     
  Partials     2113     2113

Flag	Coverage Δ
javascript	`55.77% <25.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...d/packages/superset-ui-chart-controls/src/index.ts	`100.00% <ø> (ø)`
...-chart-controls/src/sections/advancedAnalytics.tsx	`14.28% <ø> (ø)`
...rset-ui-chart-controls/src/sections/chartTitle.tsx	`100.00% <ø> (ø)`
...legacy-plugin-chart-partition/src/controlPanel.tsx	`25.00% <ø> (ø)`
...gins/legacy-plugin-chart-rose/src/controlPanel.tsx	`50.00% <ø> (ø)`
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx	`100.00% <ø> (ø)`
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx	`16.66% <ø> (ø)`
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx	`66.66% <ø> (ø)`
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx	`66.66% <ø> (ø)`
...ns/plugin-chart-echarts/src/Graph/controlPanel.tsx	`21.42% <ø> (ø)`
... and 15 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

...rset-frontend/packages/superset-ui-chart-controls/src/components/ControlSubSectionHeader.tsx

(cherry picked from commit 22a0fe5)

Back in #24736, I tried to make form labels(headers) not use `text-transform: uppercase;`, but somehow it got reverted. Here I'm trying to do the same and maybe push further into tab headers.

pull-request-size bot added size/M size/L and removed size/M labels Jul 18, 2023

mistercrunch force-pushed the control_subsection branch from 97478a3 to d86dd78 Compare July 18, 2023 23:37

mistercrunch requested a review from rusackas July 18, 2023 23:46

rusackas reviewed Jul 18, 2023

View reviewed changes

rusackas approved these changes Jul 19, 2023

View reviewed changes

mistercrunch mentioned this pull request Jul 19, 2023

Control subsection preset-io/superset#583

Closed

michael-s-molina reviewed Jul 20, 2023

View reviewed changes

...rset-frontend/packages/superset-ui-chart-controls/src/components/ControlSubSectionHeader.tsx Outdated Show resolved Hide resolved

mistercrunch added 8 commits July 20, 2023 14:54

chore: make control panel sub sections look better

8053b44

progress

a2414ae

more progress

475c553

notouch package-lock

d2abc0e

double import

83b2b4e

removing the <hr/>

06434af

lints

f8e53d4

removing font-transform

e03c810

mistercrunch force-pushed the control_subsection branch from f48e883 to e03c810 Compare July 20, 2023 21:55

changing import statements to clear warnings

494b9e8

mistercrunch merged commit 22a0fe5 into apache:master Jul 20, 2023

michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 21, 2023

michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023

chore: make control panel sub sections look better (#24736)

299dbb7

(cherry picked from commit 22a0fe5)

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

mistercrunch mentioned this pull request Jul 23, 2024

style: remove uppercase from labels, buttons, tabs to align with design system #29673

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: make control panel sub sections look better #24736

chore: make control panel sub sections look better #24736

mistercrunch commented Jul 18, 2023

rusackas Jul 18, 2023

mistercrunch Jul 20, 2023

rusackas left a comment

michael-s-molina commented Jul 19, 2023

kasiazjc commented Jul 19, 2023 •

edited

Loading

michael-s-molina commented Jul 19, 2023

john-bodley commented Jul 19, 2023 •

edited

Loading

kasiazjc commented Jul 19, 2023

mistercrunch commented Jul 19, 2023 •

edited

Loading

mistercrunch commented Jul 19, 2023 •

edited

Loading

mistercrunch commented Jul 19, 2023

codecov bot commented Jul 19, 2023 •

edited

Loading

chore: make control panel sub sections look better #24736

chore: make control panel sub sections look better #24736

Conversation

mistercrunch commented Jul 18, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

after

rusackas Jul 18, 2023

Choose a reason for hiding this comment

mistercrunch Jul 20, 2023

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

michael-s-molina commented Jul 19, 2023

kasiazjc commented Jul 19, 2023 • edited Loading

michael-s-molina commented Jul 19, 2023

john-bodley commented Jul 19, 2023 • edited Loading

kasiazjc commented Jul 19, 2023

mistercrunch commented Jul 19, 2023 • edited Loading

mistercrunch commented Jul 19, 2023 • edited Loading

mistercrunch commented Jul 19, 2023

codecov bot commented Jul 19, 2023 • edited Loading

Codecov Report

kasiazjc commented Jul 19, 2023 •

edited

Loading

john-bodley commented Jul 19, 2023 •

edited

Loading

mistercrunch commented Jul 19, 2023 •

edited

Loading

mistercrunch commented Jul 19, 2023 •

edited

Loading

codecov bot commented Jul 19, 2023 •

edited

Loading