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.

Sign up for GitHub

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

Jump to bottom

fix(explore): dnd multiple columns doesn't work #15781

Merged
merged 1 commit into from
Jul 19, 2021
Merged

fix(explore): dnd multiple columns doesn't work #15781

merged 1 commit into from
Jul 19, 2021

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Jul 19, 2021 •
edited
Loading

SUMMARY

Due to recent changes, drag and dropping metrics or columns in controls that accept multiple values did not work correctly - instead of adding a new value, the first was replaced. This PR fixes that behaviour.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
https://user-images.githubusercontent.com/15073128/126191515-85cca674-e237-4b60-97f9-6a383a61718e.mov

After:
https://user-images.githubusercontent.com/15073128/126191555-b43c723e-7c60-45f6-90c0-7a6e7c1b82ea.mov

TESTING INSTRUCTIONS

  1. Enable drag and drop feature flag
  2. Open a chart that has controls that accept multiple values (e.g. Line chart's Metrics control)
  3. Add multiple values

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

CC @junlincc

@michael-s-molina
Copy link
Member

@kgabryje Should other places that use props.multi be changed to also consider props.isMulti?

suddjian
suddjian reviewed Jul 19, 2021
View reviewed changes
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx Outdated
@@ -162,6 +162,7 @@ export const DndMetricSelect = (props: any) => {

const onNewMetric = (newMetric: Metric) => {
const newValue = props.isMulti ? [...value, newMetric] : [newMetric];
props.isMulti || props.multi ? [...value, newMetric] : [newMetric];
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this line will do anything because the result is not being assigned to a variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, mistake while formatting. Fixed now

@codecov
Copy link

codecov bot commented Jul 19, 2021 •
edited
Loading

Codecov Report

Merging #15781 (4a8e48f) into master (45c3ae0) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #15781   +/-   ##
=======================================
  Coverage   77.07%   77.07%           
=======================================
  Files         983      983           
  Lines       51618    51618           
  Branches     6991     6991           
=======================================
  Hits        39785    39785           
  Misses      11608    11608           
  Partials      225      225
Flag Coverage Δ
javascript 71.69% <0.00%> (ø)

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

Impacted Files Coverage Δ
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx 44.44% <0.00%> (ø)
...c/views/CRUD/data/database/DatabaseModal/index.tsx 47.57% <0.00%> (ø)

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 45c3ae0...4a8e48f. Read the comment docs.

@kgabryje
Copy link
Member Author

@michael-s-molina I double checked and it seems that isMulti isn't accepted by that component at all, so I changed it to check only props.multi. Thanks for spotting that!

@kgabryje kgabryje merged commit 239336c into apache:master Jul 19, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@kgabryje
fix(explore): dnd multiple columns doesn't work (apache#15781)
4b7ad7f
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@kgabryje
fix(explore): dnd multiple columns doesn't work (apache#15781)
337eea3
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@kgabryje
fix(explore): dnd multiple columns doesn't work (apache#15781)
4248a8b
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suddjian suddjian suddjian left review comments

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@zhaoyongjie zhaoyongjie Awaiting requested review from zhaoyongjie

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kgabryje @michael-s-molina @suddjian @mistercrunch