feat(listviews): SIP-34 Bulk Select #10298
Conversation
nytai
force-pushed
the
tai/SIP-34-bulk-select
branch
from
36f1d3d to
ceeab9c
Compare
superset-frontend/spec/javascripts/components/ListView/ListView_spec.jsx
Show resolved
Hide resolved
| export default function Button(props) { | ||
| const buttonProps = { ...props }; | ||
| const SupersetButton = styled(BootstrapButton)` | ||
| &.supersetButton { |
There was a problem hiding this comment.
Think we need to have the .supersetButton class here anymore, or should these be the default styles for SupersetButton?
There was a problem hiding this comment.
This button is used in other areas of the product, so there's potential for breakage. Adding the .supersetButton class renders the button as a wide CTA button (as per the SIP-34 designs). We could abstract this into a prop and use that instead, something like style: 'bootstrap' | 'default' | 'small' | 'link' | ... to control these styles, where bootstrap defaults to the bootstrap styles, and the others pertain to the new styling.
There was a problem hiding this comment.
I like that idea... then ALL the buttons usage can be migrated to import/use the new Button component, instead of direct (React-)Bootstrap use, and they'll be easy to find/replace as we go forward with SIP-34.
| </span> | ||
| <div className="divider" /> | ||
| {bulkActions.map(action => ( | ||
| <Button |
There was a problem hiding this comment.
Should this be an instance of SupersetButton?
Also, I'm assuming the margin-left style applied is to add a gap between buttons. If so, maybe we can kill off the margin-left style in this CSS, and add it to the SupersetButton component, e.g.:
margin-left: ${({ theme }) => theme.gridUnit * 4}px;
&:first-of-type {
margin-left: 0;
}
There was a problem hiding this comment.
n/m the comment about the SupersetButton instance... didn't realize that's what's exported by Button
There was a problem hiding this comment.
I'll give that styling a shot, originally I has the margins on the actual button component, but it seemed like there's more flexibility in the button not assuming anything about it's surrounding.
There was a problem hiding this comment.
re: the SupersetButton comment, do you think it makes more sense to just make a new SupersetButton component and make the split between the new and the old explicit, or does having one component serving both make sense?
There was a problem hiding this comment.
Probably debatable, but my gut tells me that one component serving both makes sense, perhaps with some props to drive it as you've suggested. Then all the code is in one spot, and easier to maintain/migrate/style, especially if/when we shift into AntD.
| dashboardCount: number; | ||
| loading: boolean; | ||
| dashboards: any[]; |
There was a problem hiding this comment.
Not sure if it's worth the effort, but if instead of dashboard____ keys, these could be made more generic, e.g. itemCount, and then ListView could export a shared/reusable interface?
There was a problem hiding this comment.
that's an interesting idea.
There was a problem hiding this comment.
DatasetList got converted into a functional component with hooks, so the shared interface makes less sense. Shared hooks on the other hand.... 🤔
|
Added some questions/comments/etc, but this is looking great! |
Codecov Report
@@ Coverage Diff @@
## master #10298 +/- ##
==========================================
+ Coverage 70.60% 70.65% +0.05%
==========================================
Files 600 601 +1
Lines 32182 32269 +87
Branches 3257 3275 +18
==========================================
+ Hits 22721 22801 +80
- Misses 9358 9362 +4
- Partials 103 106 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
|
||
| const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' }; | ||
|
|
||
| export default function Button(props) { | ||
| const buttonProps = { ...props }; | ||
| const SupersetButton = styled(BootstrapButton)` |
There was a problem hiding this comment.
How is this styling different from /views/datasetList/Button? Should these two components be joined together?
cc @lilykuang
There was a problem hiding this comment.
yes, they are very similar. They should probably be joined together.
There was a problem hiding this comment.
@nytai were you planning to action on this? Or should I review again now?
nytai
force-pushed
the
tai/SIP-34-bulk-select
branch
from
5854c53 to
b35adea
Compare
…fetch after add behavior
nytai
force-pushed
the
tai/SIP-34-bulk-select
branch
from
b35adea to
c5f2929
Compare
| @@ -104,8 +104,16 @@ | |||
| } | |||
|
|
|||
| .table-row { | |||
| .actions { | |||
| opacity: 0; | |||
There was a problem hiding this comment.
Awwww, didn't like the animation?
transition: opacity @timing-normal;
^I'm tempted to add this kind of thing lots of places in Superset ;)
| } | ||
| `; | ||
|
|
||
| const Button: React.FunctionComponent<ButtonProps> = props => { |
There was a problem hiding this comment.
instead of using arrow functions like this, I think it's better to do:
export default function Button(props: ButtonProps) {
...The arrow function results in an anonymous function and makes stack traces harder to read
There was a problem hiding this comment.
That's a good point, though React.FunctionComponent<ButtonProps> adds the react builtin props/children prop.
|
Impacts #8976 |
mistercrunch
added
🏷️ bot
SUMMARY
supersetButton)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Datasets
Charts
Dashboards
TEST PLAN
ADDITIONAL INFORMATION
ENABLE_REACT_CRUD_VIEWSconfig flag