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

Jump to bottom

feat: massive theming-related PR #31590

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

feat: massive theming-related PR #31590

wants to merge 7 commits into from
+5,552 −2,308

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Dec 20, 2024 •
edited
Loading

Excuse the large PR, but this is fairly tangled up, and working on fixing up theming probably requires some massive PRs as it's really hard to proceed PR-by-PR - especially during the holiday break...

Introducing less handlebars templates

First. Clearly we should move away from less and commit to emotion/antd for theming Now deleting the less files is going to be difficult. In the meantime, I wanted to provide a way for less files to source from the main theme object. I decided to go with handlebars since that should be part of the build process.

Considerations:

  • this is temporary, goal is to get rid of less altogether. Having this enables layering the theming work, and allows to make the theme DRY, instead of referncing the same colors a bunch of times
  • eventually could remove the .less files from the repo, and make sure they are dynamically generated on every builds. In the meantime I thought I'd leave them here, and we can instruct people to alter .less.hds files and run npm run compile-less on demand, if/when altering the main theme

Large refactor - what's in this PR?

  • less templates - as described above
  • New Theme object and subpackage as commented in the theming SIP
  • Refactoring/fixing out-of-theme, css-heavy components
    • Alert - vanilla is better!
    • AceEditor
    • EmptyState refactor (that component WAS A MESS, felt almost like on-purpose maze of indirections), fixed the svg so it can receive currentColor as opposed to using an tag - required for making it themable/dark-modable
    • ErrorAlert refactor (also a huge mess, simplifying quite a bit here)
  • Making antd components more vanilla
  • Getting rid of a bunch of CSS
  • Start aligning our css-in-js across the app to use the antd theme tokens
  • storybook improvements
    • improving the theme stories

what's NOT in this PR? - yet to come

  • more theme.antd referencing in emotion
  • move CSS / LESS deletion
  • antd v5 migrations
  • ability to configure an antd ThemeConfig as the theme setup, this will require more moving from legacy theme object to antd

@korbit-ai Korbit AI
Copy link

korbit-ai bot commented Dec 20, 2024

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

@rusackas rusackas self-requested a review December 20, 2024 23:27
@mistercrunch mistercrunch force-pushed the template_less branch 2 times, most recently from f1c7929 to 43452ff Compare December 24, 2024 04:49
@mistercrunch mistercrunch changed the title feat: feat(less): templatize .less files to reference main theme feat: feat(less): massive theming-related PR Dec 30, 2024
@mistercrunch mistercrunch changed the title feat: feat(less): massive theming-related PR feat: massive theming-related PR Dec 30, 2024
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Jan 5, 2025
@mistercrunch mistercrunch mentioned this pull request Jan 6, 2025
Closed
@mistercrunch mistercrunch force-pushed the template_less branch 2 times, most recently from 003f992 to 1652452 Compare January 9, 2025 07:07
mistercrunch added a commit that referenced this pull request Jan 14, 2025
@mistercrunch
chore: add eslint to pre-commit hooks
1b57a1d 
Chiseling at #31590 and bringing what's atomically committable out of there.

This simply adds eslint checks to pre-commit. Note that:
- it requires having run `npm i` in superset-frontend
- it's set up to NOT run in CI as part of the pre-commit validation workflow, since we run eslint more formally in another workflow

Why doing this? Currently it's common to forget to run `npm run lint` prior to committing/pushing, so people can waste time waiting for CI to fail where it could be caught easily. It's nice to have pre-commit do the check itself because it will only evaluate the files that have changed, making it much faster than running a full lint run against all files.
@mistercrunch mistercrunch mentioned this pull request Jan 14, 2025
Merged
mistercrunch added a commit that referenced this pull request Jan 14, 2025
@mistercrunch
chore: properly import expect from chai in cypress-base/cypress/suppo…
cf56599 
…rt/e2e.ts

While working on #31590, I noticed that `expect` was not properly imported. It was using it from global for some unknown reason.
@mistercrunch mistercrunch mentioned this pull request Jan 14, 2025
Merged
@github-actions github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Jan 14, 2025
mistercrunch added a commit that referenced this pull request Jan 15, 2025
@mistercrunch
chore: refactor Alert-related components
82f782c 
Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD.

Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5
@mistercrunch mistercrunch mentioned this pull request Jan 15, 2025
Open
mistercrunch added a commit that referenced this pull request Jan 16, 2025
@mistercrunch
chore: refactor Alert-related components
b29aece 
Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD.

Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5
mistercrunch added a commit that referenced this pull request Jan 16, 2025
@mistercrunch
chore: refactor Alert-related components
d381285 
Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD.

Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5
mistercrunch added a commit that referenced this pull request Jan 18, 2025
@mistercrunch
chore: refactor Alert-related components
633c619 
Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD.

Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5
mistercrunch added a commit that referenced this pull request Jan 18, 2025
@mistercrunch
chore: refactor Alert-related components
9b9531a 
Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD.

Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rusackas rusackas Awaiting requested review from rusackas rusackas is a code owner

@sadpandajoe sadpandajoe Awaiting requested review from sadpandajoe sadpandajoe will be requested when the pull request is marked ready for review sadpandajoe is a code owner

@geido geido Awaiting requested review from geido geido will be requested when the pull request is marked ready for review geido is a code owner

@eschutho eschutho Awaiting requested review from eschutho eschutho will be requested when the pull request is marked ready for review eschutho is a code owner

@betodealmeida betodealmeida Awaiting requested review from betodealmeida betodealmeida will be requested when the pull request is marked ready for review betodealmeida is a code owner

@michael-s-molina michael-s-molina Awaiting requested review from michael-s-molina michael-s-molina will be requested when the pull request is marked ready for review michael-s-molina is a code owner

@kgabryje kgabryje Awaiting requested review from kgabryje kgabryje will be requested when the pull request is marked ready for review kgabryje is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
dependencies:npm packages plugins preset-io review:draft size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mistercrunch @sadpandajoe