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

style: DOCTYPE tag, and related CSS cleanup/refactoring #10302

Merged
merged 17 commits into from Jul 30, 2020

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Jul 13, 2020

SUMMARY

Superset has been running in quirks mode due to a lack of DOCTYPE tag. This adds the DOCTYPE, which causes various CSS issues. This code also fixes that CSS. CSS fixes led to some Emotion styles, removing some JS-calculated dimensions, leaning on Flexbox more, and removing at least one Bootstrap Panel component. Hopefully we can continue to simplify and speed up rendering using these techniques. UI should remain effectively unchanged.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Made a change along the way, to have control panels scroll within the control tabs, so you don't have to scroll all the way up to switch tabs. Been meaning to do this forever anyway. Here's the AFTER gif:
scroll

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@rusackas rusackas changed the title DOCTYPE tag, and related CSS cleanup/refactoring style: DOCTYPE tag, and related CSS cleanup/refactoring Jul 13, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #10302 into master will increase coverage by 4.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10302      +/-   ##
==========================================
+ Coverage   65.46%   69.65%   +4.18%     
==========================================
  Files         603      196     -407     
  Lines       32436    19060   -13376     
  Branches     3297        0    -3297     
==========================================
- Hits        21234    13276    -7958     
+ Misses      11018     5784    -5234     
+ Partials      184        0     -184     
Flag Coverage Δ
#javascript ?
#python 69.65% <ø> (-0.11%) ⬇️

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

Impacted Files Coverage Δ
superset/views/database/mixins.py 59.64% <0.00%> (-21.06%) ⬇️
superset/db_engine_specs/mysql.py 78.72% <0.00%> (-12.77%) ⬇️
superset/views/database/validators.py 78.94% <0.00%> (-5.27%) ⬇️
superset/views/sql_lab.py 58.62% <0.00%> (-3.96%) ⬇️
superset/databases/api.py 84.61% <0.00%> (-2.89%) ⬇️
superset/db_engine_specs/postgres.py 97.36% <0.00%> (-2.64%) ⬇️
superset/views/database/views.py 61.32% <0.00%> (-1.66%) ⬇️
superset/models/sql_lab.py 91.12% <0.00%> (-1.62%) ⬇️
superset/models/core.py 85.27% <0.00%> (-1.39%) ⬇️
superset/jinja_context.py 80.00% <0.00%> (-0.96%) ⬇️
... and 419 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 3c39b26...3837b00. Read the comment docs.

@nytai nytai marked this pull request as ready for review July 29, 2020 23:45
Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

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

LGTM. This is a much needed change!

@nytai nytai merged commit 16459ad into apache:master Jul 30, 2020
@nytai nytai deleted the doctype branch July 30, 2020 01:49
align-content: stretch;
div:last-of-type {
flex-basis: 100%;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is breaking FilterBox in Explore page. @rusackas

Copy link
Member Author

Choose a reason for hiding this comment

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

Dang! Thanks for the quick catch. I'll take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't get to fix it entirely last night... but it's not just that style... there are some dimensions that need to be added to a div... they're being added to other viz components, but not this one for some reason. Hope to have a PR soon.

<Styles className="chart-container">
<div>{header}</div>
<div>{this.renderChart()}</div>
</Styles>
Copy link
Member

@ktmud ktmud Aug 3, 2020

Choose a reason for hiding this comment

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

@rusackas by not using the Panel component, you also missed some CSS styles for .panel-heading and .panel-body.

After

image

Before

image

Copy link
Member

Choose a reason for hiding this comment

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

Just pushed a fix. I'm using the raw boostrap classes for now. Let's refactor it into Antd components or a more self-contained styled component in the future.

@ktmud ktmud mentioned this pull request Aug 3, 2020
6 tasks
@willbarrett
Copy link
Member

FYI this PR broke thumbnails for alerts #10488

@rusackas
Copy link
Member Author

Impacts #8976

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 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.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants