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

Font styling consolidation #8633

Merged
merged 32 commits into from
Nov 27, 2019

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Nov 22, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This PR is attempting to align/consolidate the basic stylistic parameters of typography for Superset, using variables to provide font weights, font sizes, and line heights.

A few bits of unused CSS were also cleaned up in here. A few lingering colors were also brought in line with the new color variables as well.

Some compromises were made nudging a size this way or that, but the general look and feel of the UI should remain essentially unchanged.

SCREENSHOTS

A couple of header sizes, like above the illustration of the babies, and the title of the dashboard, seen here, are probably the most egregious size changes. Most everything else should be relatively unnoticeable, to the best of my knowledge/ability.

Old:
image

New:
image

Old:
old_dropdown

New:
newnew-dropdown

Old:
old-buttons

New:
newnew-buttons

Old:
image

New:
image

REVIEWERS

@mistercrunch @etr2460

@codecov-io
Copy link

codecov-io commented Nov 22, 2019

Codecov Report

Merging #8633 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8633   +/-   ##
=======================================
  Coverage   65.75%   65.75%           
=======================================
  Files         482      482           
  Lines       23825    23825           
  Branches     2594     2594           
=======================================
  Hits        15667    15667           
  Misses       7985     7985           
  Partials      173      173
Impacted Files Coverage Δ
...rset/assets/src/SqlLab/components/TableElement.jsx 83.52% <ø> (ø) ⬆️
superset/examples/css_templates.py 100% <ø> (ø) ⬆️

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 96fb108...5e78695. Read the comment docs.

@rusackas rusackas marked this pull request as ready for review November 25, 2019 22:12
@rusackas rusackas changed the title WiP: Font styling consolidation Font styling consolidation Nov 25, 2019
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

can you add some screenshots of examples where stuff changed?

@@ -17,7 +17,7 @@
"build": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=production webpack --mode=production --colors --progress",
"lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx . && tslint -c tslint.json ./{src,spec}/**/*.ts{,x}",
"lint-fix": "eslint --fix --ignore-path=.eslintignore --ext .js,.jsx . && tslint -c tslint.json --fix ./{src,spec}/**/*.ts{,x} && npm run clean-css",
"clean-css": "prettier --write src/**/*.{css,less,sass,scss}",
"clean-css": "prettier --write 'src/**/*.{css,less,sass,scss}'",
Copy link
Member

Choose a reason for hiding this comment

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

does a linter catch if css isn't formatted properly with this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at this point... was just using it to make life easier and catch myself. Does your Prettier PR address that? Don't want to duplicate effort here :)

superset/assets/src/CRUD/crud.less Show resolved Hide resolved
superset/assets/stylesheets/less/variables.less Outdated Show resolved Hide resolved
@rusackas
Copy link
Member Author

can you add some screenshots of examples where stuff changed?

I added a couple screenshots to the overview at the top. The most noticeable bits are that the dashboard title shrunk a teensy bit, and in-widget H1 tags got a little larger. Almost everything is negligible.

There were a couple instances of a font being set to 15px in size, which I would track down and bump to the standardized sizes of 14px or 16px, whichever looked more consistent in context. I think the changes were negligible, but I may be able to track them all down and add the various screenshots if anyone is particularly nervous about it.

@graceguo-supercat
Copy link

can you also paste a screenshot of active and inactive button?

@rusackas
Copy link
Member Author

can you also paste a screenshot of active and inactive button?

Hi @graceguo-supercat :) I added some of the main buttons in the description above. There are slight color discrepancies, but those I believe are addressed in another PR. Let me know if there are other buttons/features you'd like to see, but I believe the visible impact of this PR is pretty tiny.

@rusackas
Copy link
Member Author

@etr2460 would you mind taking another look at this one? If there's nothing objectionable in here, I'd love to sneak it in before the big Prettier PR ;)

@mistercrunch mistercrunch merged commit 172b90e into apache:master Nov 27, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 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/L 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants