refactor: Bootstrap to AntD - Button #12832
Conversation
michael-s-molina
force-pushed
the
bootstrap-to-antd-button
branch
from
ba7e472 to
144b26d
Compare
Codecov Report
@@ Coverage Diff @@
## master #12832 +/- ##
==========================================
- Coverage 65.05% 63.40% -1.65%
==========================================
Files 1021 488 -533
Lines 50095 30173 -19922
Branches 5141 0 -5141
==========================================
- Hits 32587 19132 -13455
+ Misses 17332 11041 -6291
+ Partials 176 0 -176
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/spec/javascripts/addSlice/AddSliceContainer_spec.tsx
Outdated
Show resolved
Hide resolved
| import Button from 'src/components/Button'; | ||
| import ButtonGroup from './index'; | ||
|
|
||
| export default { |
There was a problem hiding this comment.
Maybe a minor thing, but I kind of prefer named exports to default exports. I'm not sure where this pattern is going to settle in Superset, but I like that named imports are typically easier to search for in the codebase.
There was a problem hiding this comment.
@rusackas In this specific case I'm using default export following Storybook documentation:
The default export defines metadata about your component, including the component itself, its title (where it will show up in the navigation UI story hierarchy), decorators, and parameters.
Now looking at the rest of the application, I prefer named exports also but currently the majority of our components are using default exports and the screens using them are importing using component's folder name. Let me describe here how I'm going to tackle this:
- First I'm going to remove the difference between common components and components. All will be components with their own folder, storybook and test. For this step, I will preserve current standard and use default exports.
- Remove Bootstrap dependency
- After all components are following the same structure, we will create an
indexin components folder exporting our version of the components using named exports. That way we'll import our version usingimport { Component } from 'src/components'; - Changing the code to use named exports will be done in a specific PR.
| options: { | ||
| Superset: 'http://https://superset.apache.org/', | ||
| None: null, | ||
| superset: 'https://superset.apache.org/', |
There was a problem hiding this comment.
I have no strong feelings about the capitalization here, but I wonder if we should stick to the Superset caps rules (Sentence case) or if there's a rationale behind switching to lower case.
There was a problem hiding this comment.
Here I made a conscious change because those are properties and should appear as such in controls panel. It's the same as reading an API. In fact, if we don't specify names for properties, Storybook will automatically create them using lower case.
superset-frontend/src/components/ButtonGroup/ButtonGroup.stories.jsx
Outdated
Show resolved
Hide resolved
| @@ -296,8 +296,8 @@ class PropertiesModal extends React.PureComponent { | |||
| footer={ | |||
| <> | |||
| <Button | |||
| type="button" | |||
| buttonSize="sm" | |||
| htmlType="button" | |||
There was a problem hiding this comment.
This seems fine, but I'm curious if/why we need it.
There was a problem hiding this comment.
Me too 😄! I just changed type to htmlType. I think the developer wanted to make it clear that he's handling the submit.
| onClick={onSave} | ||
| data-test="query-save-button" | ||
| <div | ||
| css={{ |
There was a problem hiding this comment.
this is fine by me, but curious as to why you prefer the <div css={{... approach over the styled.div approach. They both seem to meet the same goal, so just wondering :D
There was a problem hiding this comment.
For me the use of a property seems more natural to the objective of customizing a component. Here you will find a detailed explanation but to summarize:
- You’re still writing regular React components
- Object styles are easier to work with
- Naming things is hard
- You colocate styles with elements
- Composition is dead easy
| <IconContainer> | ||
| <Icon name="plus-small" /> | ||
| <div> | ||
| <i className="fa fa-plus" /> |
There was a problem hiding this comment.
A little sad about moving away from the custom icon to the font-awesome one, but I see why you're trying to whittle out the weird div.
@geido should we just not worry about this for now, and tackle it after your icon work goes through?
There was a problem hiding this comment.
Ok. Here is the problem. We have a lot of places where a button is defined with a nested <i> element referencing an icon. These icons don't have any margin or padding, it's the real icon size. That's why this code here in Button automatically creates a margin between the icon and the text:
'i:first-of-type, svg:first-of-type': {
marginRight: theme.gridUnit * 2,
padding: `0 ${theme.gridUnit * 2} 0 0`,
},
The Icon component have a predefined viewBox of 24x24, so that margin is not required. I could replace all <i> elements with the Icon component and remove the margin but as you mentioned @geido is already working on that. So I think the safest path is to make this modification in Icon PR and also remove Button automatic margin there.
| <Icon name="plus-small" /> Dashboard{' '} | ||
| </IconContainer> | ||
| <div> | ||
| <i className="fa fa-plus" /> Dashboard{' '} |
There was a problem hiding this comment.
Same here... would prefer to use our custom icons if possible, but those are being revamped presently
There was a problem hiding this comment.
Same as previous comment.
| .slice(0, tableName.length - 1) | ||
| .join('')} | ||
| <i className="fa fa-plus" /> | ||
| {tableName === 'SAVED_QUERIES' |
There was a problem hiding this comment.
Same as previous comment.
michael-s-molina
force-pushed
the
bootstrap-to-antd-button
branch
from
144b26d to
f03dedc
Compare
There was a problem hiding this comment.
Thanks for the clarifications @michael-s-molina - I think we can merge this as-is and then sweep through all buttons containing icons for consistency once @geido 's Icon PR is also merged.
junlincc
added
risk:migration
mistercrunch
added
🏷️ bot
SUMMARY
Buttoncomponent from Bootstrap to AntD.default,smallandxsmall).ButtonStorybook to use typescript.2x grid-unitinstead of4x grid-unitin listings following the same style as other parts of the application (modals, query editor, etc).ButtonGroupwith internal component.ButtonGroupstorybook.ButtonGrouptest suite.See: #10254
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
@rusackas @junlincc All changes described here had to be made in a single PR because they are all correlated.
TEST PLAN
1 - Open any screen that contains a button (explore, welcome, dashboards, etc.)
2 - All buttons should have the same theme and behavior
3 - Additional testing can be done using Storybook
ADDITIONAL INFORMATION