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: dark filter popover background #11611

Merged
merged 8 commits into from
Nov 19, 2020

Conversation

rusackas
Copy link
Member

@rusackas rusackas commented Nov 7, 2020

SUMMARY

Adds an inverted (dark) color theme to the antd popover used in the filter status display.

Edit: adjusted some spacing, added the section header colors, tidied up some selectors that were applying the white font color.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TEST PLAN

Manual visual confirmation 👀

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

}
.ant-popover > .ant-popover-content > .ant-popover-arrow{
border-top-color: ${theme.colors.grayscale.dark2}cc;
border-left-color: ${theme.colors.grayscale.dark2}cc;
Copy link
Member

Choose a reason for hiding this comment

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

This would need to be more sophisticated to deal with popovers that open upwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done and done.

@suddjian
Copy link
Member

suddjian commented Nov 9, 2020

According to emotion's docs on the <Global> component:

Global styles are also removed when the styles change or when the Global component unmounts.

So to get dark popovers to work without interfering with light popovers on the same page, we have to get <Global> to unmount. I guess antd is rendering the content even when the popover is not open, so that's strange.

Anyway, whatever hacks we do to make this work, we should document it and link to an issue on the Antd

@rusackas rusackas force-pushed the dark-filter-popover branch from 383debe to 065d3f5 Compare November 17, 2020 05:04
@codecov-io
Copy link

codecov-io commented Nov 17, 2020

Codecov Report

Merging #11611 (6a3a35a) into master (c79dc47) will increase coverage by 0.87%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11611      +/-   ##
==========================================
+ Coverage   66.24%   67.11%   +0.87%     
==========================================
  Files         897      897              
  Lines       43463    43466       +3     
  Branches     4186     4187       +1     
==========================================
+ Hits        28790    29174     +384     
+ Misses      14557    14188     -369     
+ Partials      116      104      -12     
Flag Coverage Δ
cypress 55.34% <0.00%> (+6.55%) ⬆️
javascript 62.82% <50.00%> (-0.01%) ⬇️
python 63.31% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...d/src/dashboard/components/FiltersBadge/Styles.tsx 65.78% <0.00%> (-3.66%) ⬇️
...dashboard/components/FiltersBadge/DetailsPanel.tsx 50.00% <100.00%> (+1.16%) ⬆️
superset/db_engine_specs/presto.py 81.64% <0.00%> (-0.65%) ⬇️
...src/explore/components/controls/MetricsControl.jsx 89.65% <0.00%> (+0.57%) ⬆️
superset-frontend/src/explore/exploreUtils.js 80.29% <0.00%> (+0.72%) ⬆️
...erset-frontend/src/components/DatabaseSelector.tsx 91.30% <0.00%> (+1.08%) ⬆️
...erset-frontend/src/SqlLab/components/SqlEditor.jsx 53.33% <0.00%> (+1.21%) ⬆️
...rontend/src/SqlLab/components/AceEditorWrapper.tsx 60.75% <0.00%> (+1.26%) ⬆️
...perset-frontend/src/components/CopyToClipboard.jsx 37.50% <0.00%> (+1.56%) ⬆️
superset-frontend/src/utils/common.js 74.19% <0.00%> (+1.61%) ⬆️
... and 47 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 c79dc47...6a3a35a. Read the comment docs.

@mistercrunch mistercrunch changed the title Style: dark filter popover background style: dark filter popover background Nov 17, 2020
@rusackas
Copy link
Member Author

According to emotion's docs on the <Global> component:

Global styles are also removed when the styles change or when the Global component unmounts.

So to get dark popovers to work without interfering with light popovers on the same page, we have to get <Global> to unmount. I guess antd is rendering the content even when the popover is not open, so that's strange.

Anyway, whatever hacks we do to make this work, we should document it and link to an issue on the Antd

Found the trick... the Popover component supports an overlayClassName prop, which slaps a class on the div slapped into the dom. That adds the scoping needed for the global styles to not bleed to other popovers.

@rusackas rusackas marked this pull request as ready for review November 17, 2020 07:14
@rusackas
Copy link
Member Author

So far, there's just one instance of a Popover with this color theme, but I think we can expand the common/shared popover component to support this styling if we need to use it elsewhere in the future.

@junlincc
Copy link
Member

can we add icon colors as well?
Screen Shot 2020-11-17 at 8 30 01 AM

@junlincc
Copy link
Member

Screen Shot 2020-11-17 at 10 31 06 PM

just viewed the PR.
im seeing padding from the top is slightly more than it from the bottom.

Copy link
Member

@suddjian suddjian left a comment

Choose a reason for hiding this comment

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

👍 very nice

@junlincc junlincc self-requested a review November 18, 2020 15:49
@junlincc
Copy link
Member

please don't merge until the implementation is matching the design, also the padding is adjusted. thanks. 🙏

@rusackas rusackas force-pushed the dark-filter-popover branch from 6f4bc9e to 6a3a35a Compare November 19, 2020 17:38
@rusackas rusackas merged commit 31eaa0c into apache:master Nov 19, 2020
@rusackas rusackas deleted the dark-filter-popover branch November 19, 2020 18:54
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* style: dark filter popover background

* Story to test and demonstrate the pitfalls of global styles

* nixing the story... not going to be reusing this anyway

* adding a class to isolate dark styles

* now supports all arrow directions

* placing at the bottom rather than bottom right

* adding colors, adjusting spacing

* linting
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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/M 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants