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

chore: Moves the stylesheets folder to the assets folder #16880

Merged

Conversation

michael-s-molina
Copy link
Member

SUMMARY

Moves the stylesheets folder to the assets folder.

This work is part of SIP-61

@jinghua-qa @junlincc

TESTING INSTRUCTIONS

1 - Navigate through the application
2 - Check that all styles are loaded correctly

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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM - not sure why the linter is unable to find the stylesheets based on the updated globs as they look totally fine to me?

Comment on lines 22 to 25
"prettier-check": "prettier --check '{src,stylesheets}/**/*.{css,less,sass,scss}'",
"prettier-check": "prettier --check '{src}/**/*.{css,less,sass,scss}'",
"lint-fix": "eslint --fix --ignore-path=.eslintignore --ext .js,.jsx,.ts,tsx . && npm run clean-css && npm run type",
"clean-css": "prettier --write '{src,stylesheets}/**/*.{css,less,sass,scss}'",
"format": "prettier --write './{src,spec,stylesheets,cypress-base}/**/*{.js,.jsx,.ts,.tsx,.css,.less,.scss,.sass}'",
"clean-css": "prettier --write '{src}/**/*.{css,less,sass,scss}'",
"format": "prettier --write './{src,spec,cypress-base}/**/*{.js,.jsx,.ts,.tsx,.css,.less,.scss,.sass}'",
Copy link
Member

Choose a reason for hiding this comment

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

The cases with '{src}/**/...' would probably look nicer as just 'src/**/...'

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the thing making the linter fail. I changed to your suggestion.

@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #16880 (6621f55) into master (b35645c) will decrease coverage by 0.07%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16880      +/-   ##
==========================================
- Coverage   77.05%   76.97%   -0.08%     
==========================================
  Files        1021     1022       +1     
  Lines       54693    54756      +63     
  Branches     7457     7472      +15     
==========================================
+ Hits        42141    42147       +6     
- Misses      12307    12361      +54     
- Partials      245      248       +3     
Flag Coverage Δ
javascript 71.04% <33.33%> (-0.15%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <ø> (ø)
...nd/src/assets/stylesheets/less/cosmo/cosmoTheme.js 0.00% <ø> (ø)
superset-frontend/src/explore/App.jsx 0.00% <ø> (ø)
superset-frontend/src/theme.ts 0.00% <0.00%> (ø)
superset-frontend/src/utils/downloadAsImage.ts 41.37% <ø> (ø)
...rset-frontend/src/components/TableLoader/index.tsx 100.00% <100.00%> (ø)
superset-frontend/src/dataMask/actions.ts 66.66% <0.00%> (-5.56%) ⬇️
.../database/DatabaseModal/DatabaseConnectionForm.tsx 48.71% <0.00%> (-5.21%) ⬇️
superset-frontend/src/dataMask/reducer.ts 68.51% <0.00%> (-2.64%) ⬇️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 93.33% <0.00%> (-0.99%) ⬇️
... and 6 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 b35645c...6621f55. Read the comment docs.

@pkdotson
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

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

@jinghua-qa
Copy link
Member

I have tested in the ephemeral environment and i saw one issue:

When i run query in sql, i did not see 'EXPLORE' option under results tab and did not see this issue in master, not sure whether is this PR related. Can you check that?

Screen Shot 2021-09-29 at 10 52 55 AM

@michael-s-molina
Copy link
Member Author

@jinghua-qa Thanks for testing! I was able to reproduce the same problem on master so it's not related to this PR. I checked and it's not related to the stylesheets. I took note here and I will fix the problem in another PR.

@michael-s-molina
Copy link
Member Author

@jinghua-qa The problem with the Explore button was fixed here #16908

Copy link
Member

@pkdotson pkdotson left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for the changes.

@michael-s-molina michael-s-molina merged commit 05632b9 into apache:master Sep 30, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
* chore: Moves the stylesheets folder to the assets folder

* Changes {src} to src
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
* chore: Moves the stylesheets folder to the assets folder

* Changes {src} to src
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 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 size/M 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants