Skip to content
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

feat: card view bulk select #10607

Merged
merged 3 commits into from
Aug 19, 2020
Merged

Conversation

lilykuang
Copy link
Member

@lilykuang lilykuang commented Aug 14, 2020

SUMMARY

  • user able to bulk select and apply bulk actions to the card view

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

updated video
https://www.loom.com/share/70d0f0419e2b4012bde0a4438e8510d3

TEST PLAN

WIP

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@mistercrunch
Copy link
Member

I think noticed on the video that the border size changes, creating some jitter. One way around this is to have the border size fixed, but going from transparent to a color.

@codecov-commenter
Copy link

Codecov Report

Merging #10607 into master will decrease coverage by 4.38%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10607      +/-   ##
==========================================
- Coverage   64.49%   60.10%   -4.39%     
==========================================
  Files         776      778       +2     
  Lines       36585    36713     +128     
  Branches     3464     3463       -1     
==========================================
- Hits        23595    22068    -1527     
- Misses      12883    14458    +1575     
- Partials      107      187      +80     
Flag Coverage Δ
#cypress ?
#javascript 60.63% <83.33%> (+<0.01%) ⬆️
#python 59.80% <ø> (-0.14%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rset-frontend/src/components/ListView/ListView.tsx 95.29% <ø> (ø)
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 76.02% <ø> (ø)
...rontend/src/views/CRUD/dashboard/DashboardList.tsx 73.64% <ø> (ø)
...rontend/src/components/ListView/CardCollection.tsx 93.75% <80.00%> (-6.25%) ⬇️
...set-frontend/src/components/ListViewCard/index.tsx 86.48% <100.00%> (+0.37%) ⬆️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 154 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03a62f1...0360e4d. Read the comment docs.

}: CardProps) {
return (
<StyledCard
cover={
<Cover>
<a href={url}>
<a href={bulkSelectEnabled ? undefined : url}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this logic up into the renderCard functions? We can make url optional and not pass it down when bulkSelect is enabled, that way this component doesn't need to know about bulk select.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we may just be able to do a

e.preventDefault();
e.stopPropagation();

in the CardWrapper's onClick handler, when bulkSelect is enabled, to prevent events from triggering on the children.

row.isSelected && bulkSelectEnabled ? 'card-selected' : ''
}
key={row.id}
onClick={() => row.toggleRowSelected()}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably check to see if bulkSelectEnabled before firing this. Like this, I think it's possible to select cards even if bulkSelect is not enabled.

@lilykuang lilykuang requested a review from nytai August 18, 2020 16:47
Copy link
Member

@nytai nytai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nytai nytai merged commit a3ac506 into apache:master Aug 19, 2020
@nytai nytai deleted the lily/card-view-bulk-select branch August 19, 2020 01:04
amitmiran137 pushed a commit to amitmiran137/incubator-superset that referenced this pull request Aug 21, 2020
* master: (43 commits)
  feat: Getting fancier with Storybook (apache#10647)
  fix: dedup groupby in viz.py while preserving order (apache#10633)
  feat: bump superset-ui for certified tag (apache#10650)
  feat: setup react page with submenu for datasources listview  (apache#10642)
  feat: add certification to metrics (apache#10630)
  feat(viz-plugins): add date formatting to pivot-table (apache#10637)
  fix: controls scroll issue (apache#10644)
  feat: Allow tests files in  /src (plus Label component tests) (apache#10634)
  fix: remove duplicated params and cache_timeout from list_columns; add viz_type to list_columns (apache#10643)
  chore: splitting button stories into separate stories (apache#10631)
  refactor: remove slice level label_colors from dashboard init load (apache#10603)
  feat: card view bulk select (apache#10607)
  style: Label styling/storybook touchups (apache#10627)
  fix: removing unsupported modal sizes (apache#10625)
  feat(datasource): remove deleted columns and update column type on metadata refresh (apache#10619)
  improve documentation for country maps (apache#10621)
  chore: npm audit fix as of 2020-08-15 (apache#10613)
  feat: dataset REST API for distinct values (apache#10595)
  chore: bump react-redux to 5.1.2, whittling console noise (apache#10602)
  fixing console error about bad html attribute (apache#10604)
  ...

# Conflicts:
#	superset-frontend/src/explore/components/ExploreViewContainer.jsx
#	superset-frontend/src/views/App.tsx
#	superset/config.py
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants