refactor: Replace usages of Popover from react-bootstrap with Antd #11163
Conversation
superset-frontend/spec/javascripts/components/URLShortLinkButton_spec.jsx
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #11163 +/- ##
==========================================
- Coverage 65.66% 61.39% -4.28%
==========================================
Files 835 836 +1
Lines 39656 39676 +20
Branches 3604 3607 +3
==========================================
- Hits 26042 24359 -1683
- Misses 13505 15136 +1631
- Partials 109 181 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -21,10 +21,6 @@ | |||
| @import '~antd/lib/style/mixins/index'; | |||
| @import '~antd/lib/style/core/base'; | |||
|
|
|||
| *[class*='ant-'] { | |||
There was a problem hiding this comment.
I removed this import, because [class='ant-'] selector gives global antd styles higher specifity than our styles and margins, font sizes etc. for some components were overriden. For example the margins of button components were reset to 0 when placed inside of popovers. I haven't found a valid use case of these styles so it seems to me that it's safe to delete them. However, I would appreciate a confirmation or a comment if we can't simply remove them.
There was a problem hiding this comment.
I think the reason this was included is so that the color variables below it in the code are inserted into the global styles. Can you confirm whether or not these colors are still being applied to elements correctly?
That said, I'm really hoping to kill off LESS variables at some point, so everything uses variables pulled in from the Emotion Theme... maybe this is the time to start looking at a better way (Emotion or otherwise) to import/use the Theme colors for all the AntD components we're now using. Thoughts?
There was a problem hiding this comment.
The global styles don't seem to use the colour variables we have defined below and it appears that the components are styled correctly.
As for moving on from LESS variables to Emotion, it sounds good, but I'd suggest creating a separate task/issue and refactoring it in a separate PR. This one is far too big already...
|
@suddjian Could you please take a look? |
kgabryje
changed the title
kgabryje
force-pushed
the
antd/popover
branch
from
3dc1302 to
1f5c47e
Compare
|
E2E tests still failing here :( |
kgabryje
force-pushed
the
antd/popover
branch
from
1f5c47e to
b83fdc1
Compare
|
@mistercrunch I fixed the tests, they should pass now. However, this PR might have some changes conflicting with the Tabs PRs, so depending which feature we merge first, the tests in the other might start failing again. I'll keep an eye on that! |
kgabryje
force-pushed
the
antd/popover
branch
from
458ae76 to
4b1dfb7
Compare
|
@mistercrunch |
… Antd (apache#11163)" This reverts commit 901a42b.
…pache#11163) * New popover component * LimitControl * Moar components migrated * TimeSeriesColumnControl * Hotkeys * ColorPicker * FilterBoxItemCOntrol * AdhocFilterEditPopover * AdhocMetric * AnnotationLayerControl * DateFilterControl * Tests fix * Fix linting issue * Fix tests * Bug fix * Test fix * Remove Antd global stylesheet * Fix linting * Fix test * Fix test * Fix test * Fix test * Fix test
mistercrunch
added
🏷️ bot
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION