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

fix(explore): disable resize bar when the results area is collapsed #21366

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

stephenLYZ
Copy link
Member

@stephenLYZ stephenLYZ commented Sep 8, 2022

SUMMARY

If the data section (results/samples) is collapsed, user can still drag the icon/object between chart and data section header to resize the chart. This behavior is confusing as in this case the data section remains collapsed and empty.

So we remove drag indicator/feature if the data section is collapsed.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

2022-09-08.10.01.47.mov

after

2022-09-08.9.58.43.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #21366 (7bd47d3) into master (1cc2148) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #21366      +/-   ##
==========================================
+ Coverage   66.47%   66.57%   +0.10%     
==========================================
  Files        1789     1791       +2     
  Lines       68381    68525     +144     
  Branches     7276     7319      +43     
==========================================
+ Hits        45455    45620     +165     
+ Misses      21051    21015      -36     
- Partials     1875     1890      +15     
Flag Coverage Δ
javascript 52.75% <100.00%> (+0.25%) ⬆️

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

Impacted Files Coverage Δ
...ntend/src/explore/components/ExploreChartPanel.jsx 71.62% <100.00%> (+4.01%) ⬆️
...s/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx 46.42% <0.00%> (-53.58%) ⬇️
...cy-preset-chart-nvd3/src/TimePivot/controlPanel.ts 33.33% <0.00%> (-16.67%) ⬇️
...t-controls/src/sections/echartsTimeSeriesQuery.tsx 33.33% <0.00%> (-16.67%) ⬇️
...perset-ui-chart-controls/src/sections/sections.tsx 87.50% <0.00%> (-12.50%) ⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx 48.43% <0.00%> (-2.83%) ⬇️
...perset-frontend/src/views/components/RightMenu.tsx 60.83% <0.00%> (-2.50%) ⬇️
superset-frontend/src/components/Select/Select.tsx 85.43% <0.00%> (-1.99%) ⬇️
...ard/components/DrillDetailPane/DrillDetailPane.tsx 81.01% <0.00%> (-1.75%) ⬇️
...trols/DateFilterControl/components/CustomFrame.tsx 70.96% <0.00%> (-1.45%) ⬇️
... and 41 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@geido
Copy link
Member

geido commented Sep 12, 2022

/testenv up

@github-actions
Copy link
Contributor

@geido Ephemeral environment spinning up at http://35.87.126.10:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

@stephenLYZ this LGTM. I am wondering if we should close the data panel automatically when it reaches the bottom, as it is in fact the same as closing it. CC @kasiazjc

Superset.mp4

@kasiazjc
Copy link
Contributor

@stephenLYZ this LGTM. I am wondering if we should close the data panel automatically when it reaches the bottom, as it is in fact the same as closing it. CC @kasiazjc

Superset.mp4

good call @geido, I think it makes sense! You cannot see anything, so there is no point in it being expanded.

There is one edge case, where someone may be playing with the drag and drop and out of the blue the section will collapse itself and they will have to manually expand which can be annoying. But I'd still say that we should go with your suggestion, Diego and we'll hear feedback that it doesn't make sense, then we'll revert it.

cc: @stephenLYZ

@stephenLYZ
Copy link
Member Author

@geido @kasiazjc Thanks for these suggestions. It makes sense. And my idea is that when we click the collapse button, we don't really collapse the panel, but rather move the panel to the bottom, we can still drag and drop the panel, but at this point dragging upwards doesn't result in a blank space, but rather the content panel, and the collapse button also reverts to its original state.

@rusackas
Copy link
Member

I feel like the suggestions being floated around might involve more edge cases and frustration than the original thing this PR fixes. Can we just merge this and ticket any additional layers of fanciness as an additional task?

@rusackas rusackas merged commit d28909d into apache:master Sep 12, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 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/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants