refactor: Bootstrap to AntD - Collapse #12920
Conversation
superset-frontend/src/explore/components/controls/FixedOrMetricControl.jsx
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #12920 +/- ##
===========================================
- Coverage 72.29% 58.55% -13.74%
===========================================
Files 864 478 -386
Lines 44883 15977 -28906
Branches 5403 4121 -1282
===========================================
- Hits 32450 9356 -23094
+ Misses 12224 6621 -5603
+ Partials 209 0 -209
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
michael-s-molina
force-pushed
the
bootstrap-to-antd-collapse
branch
2 times, most recently
from
9f1538e
to
2561ba3
Compare
michael-s-molina
force-pushed
the
bootstrap-to-antd-collapse
branch
from
2561ba3
to
98e548f
Compare
| > .ant-collapse-item.ant-collapse-no-arrow | ||
| > .ant-collapse-header { | ||
| border: 0px; | ||
| padding: 0px 0px 8px 0px; |
There was a problem hiding this comment.
| padding: 0px 0px 8px 0px; | |
| padding: 0px 0px 8px 0px; |
Nit, but would be nice to use gridUnits here and in the .well selector below.
Also, I think this deep selector is definitely better than !important, but now I'm just curious if we can tack on an id to the element itself, and use &#someID.ant-... to to throw a little more value at the selector weighting.
There was a problem hiding this comment.
Applied gridUnit.
About the ID the & is transformed into a unique class name by emotion giving the appropriate weighting value.
superset-frontend/src/common/components/Collapse/Collapse.stories.tsx
Outdated
Show resolved
Hide resolved
| )} | ||
| {this.state.type === controlTypes.metric && ( | ||
| <span> | ||
| <span style={{ fontWeight: 'normal' }}>metric: </span> |
There was a problem hiding this comment.
This looks like it would be normal font weight, and not need this style prop. But if it does need an override, it might be nice to use the css prop and pull in theme.typography.weights.normal here.
There was a problem hiding this comment.
I realize this is just stuff carried over... no need to worry here I suppose, as we'll probably audit all style attributes at some point.
There was a problem hiding this comment.
We didn't need that style. Fixed.
| > | ||
| <div className="well"> | ||
| <PopoverSection | ||
| title="Fixed" |
There was a problem hiding this comment.
Another out-of-scope item since it's not pertinent to the intent of this PR, but maybe worth adding a t() here and the "Based on a metric" title below?
There was a problem hiding this comment.
This PR is looking great in general. Most of my comments are either small nits, or tangential things I noticed that aren't part of the PR's intended effort.
The only thing holding up my approval is the export of the AntD component we don't want people using, if we're to favor the customized one.
michael-s-molina
force-pushed
the
bootstrap-to-antd-collapse
branch
from
98e548f
to
5c8879a
Compare
|
@rusackas I tackled your comments and also added tests for |
michael-s-molina
force-pushed
the
bootstrap-to-antd-collapse
branch
from
5c8879a
to
5b62a9c
Compare
Rebased. |
|
@michael-s-molina |
Fixed in #13624 |
mistercrunch
added
🏷️ bot
SUMMARY
Collapsecomponent from Bootstrap to AntD.Collapsestories into a single one with controls support.Collapsecomponent to own folder.paddingandborderin storybook gallery examples.See: #10254
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
@rusackas @junlincc @geido
TEST PLAN
1 - Open any screen that contains a Collapse (explore, welcome, dashboards, etc.)
2 - All collapse components should have the same theme and behavior
3 - Additional testing can be done using Storybook
ADDITIONAL INFORMATION