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

Draggable: only set drag handle props on the drag handle itself #50025

Merged
merged 3 commits into from Jun 2, 2022

Conversation

ashharrison90
Copy link
Contributor

@ashharrison90 ashharrison90 commented Jun 1, 2022

What this PR does / why we need it:

  • Update dependency react-select to v5.3.2 #49142 updated our version of react-select
    • this includes this pr which short circuits the onControlMouseDown function in react-select if event.defaultPrevented = true
  • react-beatiful-dnd sets event.preventDefault() on it's drag handlers
  • we were incorrectly spreading dragHandleProps onto elements that weren't the drag handles themselves, but instead the whole row.
  • changed it to spread the props onto the correct elements.
    • this also makes it so that rows can no longer be dragged from anywhere, only the appropriate drag handle.

Which issue(s) this PR fixes:

Fixes #49867

Special notes for your reviewer:

@ashharrison90 ashharrison90 added type/bug no-changelog Skip including change in changelog/release notes backport v9.0.x labels Jun 1, 2022
@ashharrison90 ashharrison90 added this to the 9.0.0-beta3 milestone Jun 1, 2022
@ashharrison90 ashharrison90 requested review from gillesdemey and a team June 1, 2022 15:52
@ashharrison90 ashharrison90 self-assigned this Jun 1, 2022
@ashharrison90 ashharrison90 requested review from joshhunt and kaydelaney and removed request for a team June 1, 2022 15:52
Copy link
Contributor

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

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

Excellent job tracking down the cause of this and coming up with a good fix! I want to demo this first to see what it's like in practice.

Not super in love with defining functions inline in (functional) components - a lot of the times it's just components within components, especially more so since the argument is called "props" 😅. Maybe we can just hoist this out to a component to this file?

@@ -93,7 +93,7 @@ export const QueryOperationRow: React.FC<QueryOperationRowProps> = ({
const actionsElement = actions && ReactUtils.renderOrCallToRender(actions, renderPropArgs);
const headerElementRendered = headerElement && ReactUtils.renderOrCallToRender(headerElement, renderPropArgs);

const rowHeader = (
const getRowHeader = (dragHandleProps?: DraggableProvidedDragHandleProps) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

is this... a... component? 😇

@joshhunt
Copy link
Contributor

joshhunt commented Jun 1, 2022

Hmmm..... In practice, I think the drag handle is quite small and awfully close to the no-confirm Delete button

image

We could give the handle a bunch of padding and offset that with negative margins (to make the drag handle flush with the outer bounds of the row), and push it a bit away from the action buttons?

image

(red bg mine, to show the target size)

quick inspector CSS:

    padding: 7px;
    margin: -9px;
    margin-left: calc(16px - 7px);

@grafanabot
Copy link
Contributor

Copy link
Member

@gillesdemey gillesdemey left a comment

Choose a reason for hiding this comment

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

Works like a charm, LGTM! :shipit:

@polibb polibb requested review from axelavargas and polibb June 2, 2022 08:52
Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

👏🏾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixed datasource UI does not work in 9x
5 participants