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(dashboard): nested rows and columns with hovered menu #12603

Merged
merged 1 commit into from Jan 21, 2021

Conversation

kkucharc
Copy link
Contributor

@kkucharc kkucharc commented Jan 19, 2021

SUMMARY

When we have nested rows and columns in the dashboard the hovered menu is not visible or have not enough space.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before
Screen_Shot_2021-01-19_at_6_52_10_AM

After
Screenshot 2021-01-19 at 17 00 30

closes #12601

TEST PLAN

  1. Go to Dashboard and pick World's Bank Data
  2. Go to edit mode
  3. Check mouse over various charts

or
Create own dashboard and nest a lot of charts and rows.

ADDITIONAL INFORMATION

@kkucharc
Copy link
Contributor Author

cc: @adam-stasiak @junlincc

@kkucharc
Copy link
Contributor Author

I have just a design question here:
if the new top margin is ok should I move also native filter icons or those should remain in same place?

Screenshot 2021-01-19 at 17 11 58

@junlincc

@junlincc junlincc added need:design-review Requires input/approval from a Designer dashboard:native-filters Related to the native filters of the Dashboard labels Jan 19, 2021
@junlincc junlincc added the hold:testing! On hold for testing label Jan 19, 2021
@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #12603 (5507398) into master (9771b82) will decrease coverage by 7.94%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12603      +/-   ##
==========================================
- Coverage   66.76%   58.81%   -7.95%     
==========================================
  Files        1015      959      -56     
  Lines       49642    46878    -2764     
  Branches     4970     4353     -617     
==========================================
- Hits        33141    27571    -5570     
- Misses      16378    19307    +2929     
+ Partials      123        0     -123     
Flag Coverage Δ
cypress 50.98% <100.00%> (ø)
javascript ?
python 63.22% <ø> (-0.75%) ⬇️

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

Impacted Files Coverage Δ
...tend/src/dashboard/components/DashboardBuilder.jsx 86.41% <100.00%> (ø)
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx 11.76% <0.00%> (-88.24%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
superset-frontend/src/components/IconTooltip.tsx 13.33% <0.00%> (-86.67%) ⬇️
...rc/dashboard/components/gridComponents/Divider.jsx 13.33% <0.00%> (-86.67%) ⬇️
...end/src/SqlLab/components/ExploreResultsButton.jsx 8.00% <0.00%> (-84.00%) ⬇️
... and 403 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 9771b82...5507398. Read the comment docs.

@adam-stasiak
Copy link
Contributor

Tested manually across safari,chrome,firefox. No issues spotted 🟢

@adam-stasiak
Copy link
Contributor

During testing I observed that extremely useful would be to have kind of more obvious graphical option to show exactly which area delete operation touches -> I mean I would love to see much bolder line because this blue one is hard to spot. What do you think @junlincc ?

@junlincc junlincc changed the title fix: nested rows and columns with hovered menu fix(dashboard): nested rows and columns with hovered menu Jan 19, 2021
@junlincc
Copy link
Member

@mihir174 Mihir, @ktmud and I have been making ad hoc design decisions more than we should for the product. I will leave above question from @adam-stasiak to you.

Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

Tested, better than before! thanks for the fix!

i wanted to rip the dashboard out lol, the entire drag & drop + move row & column + adjust chart size experience is just not intuitive(horrible🤦🏾‍♀️). we are piling fixes on a shaky foundation.

@junlincc junlincc removed the hold:testing! On hold for testing label Jan 19, 2021
@mihir174
Copy link
Contributor

@kkucharc i think that the native filter icons should remain in the same place

@junlincc junlincc removed the need:design-review Requires input/approval from a Designer label Jan 20, 2021
@kkucharc
Copy link
Contributor Author

Hi, @ktmud @villebro do you mind taking a look if those changes are ok with you?

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM and makes sense. I agree with @junlincc that some more fundamental redesign might be needed here, but in the short term this is a really good fix.

@villebro villebro merged commit effd717 into apache:master Jan 21, 2021
villebro pushed a commit that referenced this pull request Jan 25, 2021
@mistercrunch mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 dashboard:native-filters Related to the native filters of the Dashboard size/XS v1.0.1 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dashboard]Edit mode row control icons set misalignment
7 participants