chore: Migrate .less styles to Emotion #22474

kgabryje · 2022-12-20T18:00:21Z

SUMMARY

This PR migrates all .less files that target Dashboard, Explore and SqlLab. Generic stylesheets (for example with antd overrides or global LESS variables) are out of scope

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Test all flows related to Dashboard and SqlLab. Compare with current master - there shouldn't be any meaningful changes, except for small spacing changes here and there (so that the spacings use grid unit from Superset's theme).
Explore shouldn't really be affected by this PR - I removed explore/main.less stylesheet, but it seems that it wasn't even used at all. However, check the Explore page just to be safe.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

kgabryje · 2023-01-03T18:19:13Z

/testenv up

github-actions · 2023-01-03T18:21:08Z

@kgabryje Ephemeral environment spinning up at http://54.202.20.183:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

kgabryje · 2023-01-10T15:15:26Z

Thank you for great feedback @michael-s-molina! I addressed all of your comments (including translating "Empty column") except for the one about using theme fonts in Ace Editor. We can do an investigation as a follow up to verify if the comment about buggy Fira font is still valid

michael-s-molina

LGTM. Thank you for all the hard work and for addressing the comments @kgabryje. I would also suggest that you get QA's approval before merging the PR. We could schedule a team review to go through the application to see if we spot any differences as an additional precaution.

kgabryje · 2023-01-10T17:38:09Z

CC @AnushaErrabelli for review

AnushaErrabelli · 2023-01-11T02:51:06Z

LGTM.

rusackas · 2023-01-11T16:29:12Z

superset-frontend/src/SqlLab/components/App/index.jsx

+      .ant-modal-body {
+        overflow: auto;
+      }


Seems like we might be able to get rid of this one, if I recall correctly.

I'll check!

rusackas · 2023-01-11T16:30:47Z

superset-frontend/src/SqlLab/components/ResultSet/index.tsx

@@ -449,7 +456,7 @@ const ResultSet = ({
              <ButtonGroup>
                <Button
                  buttonSize="small"
-                  className="m-r-5"


If this PR doesn't do it, we can probably do a sweep to get rid of these weird little utility classes soon.

Yup, out of scope of this PR

rusackas · 2023-01-12T23:35:36Z

superset-frontend/src/assets/stylesheets/superset.less

@@ -327,6 +313,10 @@ table.table-no-hover tr:hover {
  margin-bottom: 10px;
 }

+.m-l-2 {


This probably got yanked in from another file, and i'm lost in the weeds, but just making sure we're not adding more of these utility classes if we don't need to ;)

Yeah, the "global" stylesheets were out of scope of this PR and that .m-l-2 class looked like it belonged here.

rusackas · 2023-01-12T23:37:01Z

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx

+
+    /* A row within a column has inset hover menu */
+    .dragdroppable-column .dragdroppable-row .hover-menu--left {
+      left: ${theme.gridUnit * -3}px;


Is this what we'd want if we ever changed the scale of the grid unit? If it just needs to be 1px forever and always, so be it ;)

Good point 🤔 We should do a sweep through the styles and verify if some values should be hardcoded rather than be dependent on theme

rusackas · 2023-01-12T23:39:01Z

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx

+          height: 100%;
+          top: 0;
+          left: 0;
+          z-index: 1;


At some point, we should bring back a z-index "registry" and centralize all these things values as vars.

rusackas · 2023-01-13T16:22:52Z

superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx

+    position: relative;
+
+    &.dragdroppable--dragging {
+      opacity: 0.2;


The theme contains opacity stops we should leverage wherever it makes sense. It includes 10% and 35% stops at the moment, so maybe we should try one of those here in a separate PR? There are probably random opacities all over we could audit for.

Ok, I'll see which fits better 👍

rusackas · 2023-01-13T16:24:04Z

superset-frontend/src/dashboard/components/dnd/DragDroppable.jsx

+    }
+
+    &.dragdroppable-column .resizable-container span div {
+      z-index: 10;


Again, we should have a z-index registry, so we can think of all of Superset as layers in a consolidated way, do calculations like theme.layers.navigation + 1.

rusackas · 2023-01-13T16:30:33Z

superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx

+      left: 0;
+      width: 100%;
+      height: 100%;
+      border: 2px solid ${theme.colors.primary.base};


I think this is the first 2px border I've seen in all this. Might be worth checking for consistency.

It's this border when component is focused in edit mode. Looks weird with 1px

rusackas · 2023-01-13T16:31:12Z

superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx

+    position: absolute;
+    flex-wrap: nowrap;
+    left: 1px;
+    top: -42px;


Not sure if these should be in gridUnits, or if it's -42 for a specific reason.

Yes it should be in gridUnits. It's related to component's height (40px or 10gridUnits)

superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx

rusackas · 2023-01-13T16:36:18Z

superset-frontend/src/dashboard/styles.ts

@@ -18,6 +18,39 @@
 */
 import { css, SupersetTheme } from '@superset-ui/core';

+export const headerStyles = (theme: SupersetTheme) => css`


Hmm... I wonder if these are worth moving into (or along side of) the theme itself for global re-use

Previously it targeted only Dashboard, so I moved it here in order to reduce the impact and chances of regressions. You're right though that we might want to make it global

rusackas

This is awesome! I left a few nits/questions/follow-ups for future sweeping, but in general, this seems fantastic! Should we reset the baseline on visual regression tests and make sure this doesn't trigger any alarm bells?

rusackas · 2023-01-18T17:17:25Z

@kgabryje any reason not to merge this before the inevitable conflicts arise?

kgabryje · 2023-01-19T08:17:35Z

Good call @rusackas! I'll do a sweep through your comments in a followup PR

github-actions · 2023-01-19T08:17:56Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/XXL label Dec 20, 2022

kgabryje marked this pull request as draft December 20, 2022 18:00

kgabryje force-pushed the chore/less-to-emotion branch from 782ff29 to 5e932f5 Compare December 21, 2022 17:24

kgabryje changed the title ~~[WIP] Migrate .less styles to Emotion~~ Migrate .less styles to Emotion Dec 22, 2022

kgabryje requested review from geido, jinghua-qa and michael-s-molina December 22, 2022 15:58

kgabryje changed the title ~~Migrate .less styles to Emotion~~ chore: Migrate .less styles to Emotion Dec 22, 2022

kgabryje marked this pull request as ready for review December 22, 2022 15:58

kgabryje force-pushed the chore/less-to-emotion branch 3 times, most recently from 3161f6a to 1758863 Compare December 22, 2022 16:40

kgabryje added 16 commits January 4, 2023 12:52

Move row.less, new-component.less and divider.less to Emotion

feafce8

Move chart.less to Emotion

7cad50c

Move header.less to Emotion

c127539

Move column.less to Emotion

3429628

Move markdown.less to Emotion

ed14d2e

Move builder.less to Emotion

9e548c2

Move grid.less to Emotion

65e93f7

Move dnd.less to Emotion

6bed30c

Move resizable.less to Emotion

5fa06e4

Move filter-scope-selector.less to Emotion

da91c10

Fix bug in full screen chart in dashboard

1b648f1

Move dashboard.less to Emotion

5d78e61

Fix fade not working in Emotion

453980c

Move popover-menu.less to Emotion

775e563

Remove explore/main.less file

71ba94f

Move SqlLab/main.less to Emotion

0fba068

Address code review comments

6517947

kgabryje requested review from eschutho and betodealmeida as code owners January 10, 2023 15:14

michael-s-molina approved these changes Jan 10, 2023

View reviewed changes

Lint fix

a396556

rusackas reviewed Jan 11, 2023

View reviewed changes

rusackas reviewed Jan 12, 2023

View reviewed changes

rusackas reviewed Jan 13, 2023

View reviewed changes

superset-frontend/src/dashboard/components/menu/WithPopoverMenu.tsx Show resolved Hide resolved

rusackas reviewed Jan 13, 2023

View reviewed changes

rusackas approved these changes Jan 13, 2023

View reviewed changes

kgabryje merged commit 39c96d0 into apache:master Jan 19, 2023

justinpark mentioned this pull request Feb 3, 2023

fix(sqllab): clean unwanted scrollbar #22990

Merged

9 tasks

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Migrate .less styles to Emotion #22474

chore: Migrate .less styles to Emotion #22474

kgabryje commented Dec 20, 2022 •

edited

Loading

kgabryje commented Jan 3, 2023

github-actions bot commented Jan 3, 2023

kgabryje commented Jan 10, 2023

michael-s-molina left a comment

kgabryje commented Jan 10, 2023 •

edited

Loading

AnushaErrabelli commented Jan 11, 2023

rusackas Jan 11, 2023

kgabryje Jan 19, 2023

rusackas Jan 11, 2023

kgabryje Jan 19, 2023

rusackas Jan 12, 2023

kgabryje Jan 19, 2023

rusackas Jan 12, 2023

kgabryje Jan 19, 2023

rusackas Jan 12, 2023

kgabryje Jan 19, 2023

rusackas Jan 13, 2023

kgabryje Jan 19, 2023

rusackas Jan 13, 2023

kgabryje Jan 19, 2023

rusackas Jan 13, 2023

kgabryje Jan 19, 2023

rusackas Jan 13, 2023

kgabryje Jan 19, 2023

rusackas Jan 13, 2023

kgabryje Jan 19, 2023

rusackas left a comment •

edited

Loading

rusackas commented Jan 18, 2023

kgabryje commented Jan 19, 2023

github-actions bot commented Jan 19, 2023

chore: Migrate .less styles to Emotion #22474

chore: Migrate .less styles to Emotion #22474

Conversation

kgabryje commented Dec 20, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

kgabryje commented Jan 3, 2023

github-actions bot commented Jan 3, 2023

kgabryje commented Jan 10, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

kgabryje commented Jan 10, 2023 • edited Loading

AnushaErrabelli commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas left a comment • edited Loading

Choose a reason for hiding this comment

rusackas commented Jan 18, 2023

kgabryje commented Jan 19, 2023

github-actions bot commented Jan 19, 2023

kgabryje commented Dec 20, 2022 •

edited

Loading

kgabryje commented Jan 10, 2023 •

edited

Loading

rusackas left a comment •

edited

Loading