-
Notifications
You must be signed in to change notification settings - Fork 14k
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
refactor: Boostrap to AntD - Row/Col #14100
refactor: Boostrap to AntD - Row/Col #14100
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14100 +/- ##
=======================================
Coverage 76.13% 76.13%
=======================================
Files 945 945
Lines 47927 47927
Branches 5950 5950
=======================================
Hits 36491 36491
Misses 11230 11230
Partials 206 206
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@pkdotson Ephemeral environment spinning up at http://34.220.138.107:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled the code and everything looks good to me!
09c8e07
to
061c055
Compare
@@ -114,7 +115,7 @@ export default function SaveQuery({ | |||
const renderModalBody = () => ( | |||
<FormGroup bsSize="small"> | |||
<Row> | |||
<Col md={12}> | |||
<Col xs={24}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems OK, but if there's only one Col
for each of these rows, I'm wondering if we even need the rows/cols at all. It seems like a good ol' div
would suffice.
@@ -179,7 +180,7 @@ const ScheduleQueryButton: FunctionComponent<ScheduleQueryButtonProps> = ({ | |||
</Row> | |||
{scheduleQueryWarning && ( | |||
<Row> | |||
<Col md={12}> | |||
<Col xs={24}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we even need these either. Not saying it should affect this PR, just something to consider.
@@ -87,8 +88,8 @@ export default class BoundsControl extends React.Component { | |||
<div> | |||
<ControlHeader {...this.props} /> | |||
<FormGroup bsSize="small"> | |||
<Row> | |||
<Col xs={6}> | |||
<Row gutter={16}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could use withTheme for these to get a hold of gridUnit, but it's not a blocker for this work I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, too. Noted a few things we might want to touch up, but I don't think they should prevent this from being merged.
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
Row
andCol
components from Bootstrap to AntDVizTypeControl
layout when resizingSee: #10254
@rusackas @junlincc @pkdotson
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
1.mov
1.mov
2.mov
2.mov
3.mov
3.mov
4.mov
4.mov
5.mov
5_ZdJAE5rc.mov
6.mov
6.mov
7.mov
7.mov
8.mov
8.mov
TEST PLAN
1 - I added before and after videos for each touched component
2 - Review all touched components
ADDITIONAL INFORMATION