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): drag and drop indicator UX #27558

Merged
merged 9 commits into from
Mar 27, 2024

Conversation

justinpark
Copy link
Member

@justinpark justinpark commented Mar 18, 2024

SUMMARY

The current explore drag-and-drop (DND) indicator only highlights the drop allow zone, and users can recognize the disallowed zone when they try to drop an item there. However, this UX can be challenging when the current dragging item is not droppable in the entire zone, as there is no highlighting to indicate this.

before--dnd-feedback.mov

This lack of feedback can lead users to believe that the DND functionality is not working, until they mouse over the zone.

This commit aims to address this UX issue by implementing changes. Now, all droppable zones will be highlighted when dragging, and a different color scheme will indicate which zones are available and which are unavailable for dropping.

after--dnd-feedback.mov

Additionally, the commit also updates the drag over feedback to align with the changes made to the dashboard's DND UX(#26699). This way, users can have a consistent and improved drag-and-drop experience, with clear usability feedback.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

before--explore-dnd-indicator.mov

After:

after--explore-dnd-indicator.mov

TESTING INSTRUCTIONS

Create a "Table" chart
Select RAW RECORDS and then drag the Metrics to check where is droppable zone

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

@justinpark
Copy link
Member Author

cc: @kasiazjc @yousoph for UI feedback

@mistercrunch
Copy link
Member

Looks great!

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 72.41379% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 67.51%. Comparing base (f274c47) to head (bdfb926).
Report is 38 commits behind head on master.

Files Patch % Lines
...plore/components/controls/OptionControls/index.tsx 36.36% 1 Missing and 6 partials ⚠️
...controls/DndColumnSelectControl/DndSelectLabel.tsx 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #27558      +/-   ##
==========================================
+ Coverage   67.46%   67.51%   +0.04%     
==========================================
  Files        1910     1911       +1     
  Lines       74802    74894      +92     
  Branches     8345     8371      +26     
==========================================
+ Hits        50467    50561      +94     
+ Misses      22284    22273      -11     
- Partials     2051     2060       +9     
Flag Coverage Δ
javascript 57.49% <72.41%> (+0.10%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justinpark
Copy link
Member Author

/testenv up

Copy link
Contributor

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

@rusackas rusackas requested a review from kgabryje March 19, 2024 14:52
background-color: ${({ theme, canDrop }) =>
canDrop ? theme.colors.primary.base : theme.colors.error.light1};
z-index: 10;
opacity: 0.2;
Copy link
Member

Choose a reason for hiding this comment

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

If it makes sense for these, the Superset theme has some built-in opacity stops (which we can expand on if needed)

image

display: ${({ isDragging }) => (isDragging ? 'block' : 'none')};
background-color: ${({ theme, canDrop }) =>
canDrop ? theme.colors.primary.base : theme.colors.error.light1};
z-index: 10;
Copy link
Member

Choose a reason for hiding this comment

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

There's a z-index layer for this in the Superset theme... you can uset theme.zIndex.aboveDashboardCharts

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Great

background-color: ${({ theme }) => theme.colors.primary.base};
z-index: 10;
opacity: 0.3;
top: -4px;
Copy link
Member

@rusackas rusackas Mar 19, 2024

Choose a reason for hiding this comment

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

Same for the z-index/opacity here, but also, if these positions are based on the "drop zone" having a gridUnit of space, you might want to use the gridUnit here as well for these 4px offsets. Then if someone changes the gridUnit to 6px or 2px in their theme, it'll all line up nicely :D

@rusackas
Copy link
Member

This is looking awseome! Left some CSS comments about under-utilized zindex/opacity settings, which we ought to be propagating throughout code whenever we get the chance :D

@justinpark
Copy link
Member Author

/testenv down

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Thanks for the touchups! LGTM!

@mistercrunch
Copy link
Member

wow-icegif-4

@john-bodley
Copy link
Member

@kasiazjc do you have any inputs with regards to the design change? Note this change is inline with how droppable zones function for dashboards.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Thank you for the improvement @justinpark!

isLoading || (canDrop && isOver) ? 'block' : 'none'};
background-color: ${({ theme, isLoading }) =>
isLoading ? theme.colors.grayscale.light3 : theme.colors.primary.base};
z-index: ${({ theme }) => theme.zIndex.dropdown};
Copy link
Member

Choose a reason for hiding this comment

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

Could you move the variables to the top? Like:

const TooltipSectionWrapper = styled.div`
  ${({ theme }) => css`
    font-size: ${theme.typography.sizes.s}px;

    &:not(:last-of-type) {
      margin-bottom: ${theme.gridUnit * 2}px;
    }
  `}
`;

to avoid repeating:

${({ theme, isDragging, isLoading }) => ... };

@justinpark justinpark force-pushed the fix--highlight-droppable-area branch from a24c8b9 to bdfb926 Compare March 26, 2024 17:30
@justinpark justinpark merged commit 7369754 into apache:master Mar 27, 2024
30 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@michael-s-molina michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Mar 28, 2024
michael-s-molina pushed a commit that referenced this pull request Mar 28, 2024
EandrewJones pushed a commit to UMD-ARLIS/superset that referenced this pull request Apr 5, 2024
EnxDev pushed a commit to EnxDev/superset that referenced this pull request Apr 12, 2024
@yousoph yousoph mentioned this pull request Apr 16, 2024
3 tasks
@mistercrunch mistercrunch added 🍒 4.0.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Apr 17, 2024
qleroy pushed a commit to qleroy/superset that referenced this pull request Apr 28, 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/L v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch 🍒 4.0.0 🍒 4.0.1 🍒 4.0.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants