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

Altered styles and added some props #1

Merged
merged 1 commit into from
Mar 22, 2019

Conversation

cchaos
Copy link

@cchaos cchaos commented Mar 22, 2019

While playing with the default, I've decided that the only part that needs to be automatically styled is the shading of the droppable areas (I'll also look into how users can override later).

I also felt that using panels for the droppable areas is such a useful feature that we should build it in, so I'm using the prop withPanel to add those panel styles (but not the actual component). I also added a spacing prop to both the droppable and draggable components.

In the third (custom handle) example I'm playing with the idea of using completely custom/non Panel components, like the EuiListGroup and how that's affected by the draggable.

For the copyable example, I added an EuiButtonIcon to the copied item that we should hook up to some remove function. Also, in agreement with chandler that we should allow removal, and so maybe dragging outside of the droppable area should allow removal (though this should be up to the consumer too).

Lastly, in the complex example, I changed the custom drag handle here to an EuiButtonIcon and it's not working as expected. Is it possible to allow this? I think it would simplify some hover and focus states for the consumer so they don't need to add any.

@cchaos cchaos mentioned this pull request Mar 22, 2019
9 tasks
Copy link
Owner

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Great so far. Looks like there will be another change or two?

@@ -13,26 +12,31 @@
}

&.euiDraggable--hasCustomDragHandle > .euiDraggable__item [data-react-beautiful-dnd-drag-handle] {
border: 1px solid transparent;
// What was the reason for the transparent border?
Copy link
Owner

Choose a reason for hiding this comment

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

Now that it uses euiFocusRing, there is no reason for it. Before, there was a size change in the handle when the border was added.

Copy link
Author

Choose a reason for hiding this comment

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

Cool, feel free to remove the comment then.

export interface EuiDraggableProps
extends CommonProps,
Omit<DraggableProps, 'children'> {
children: ReactElement | DraggableProps['children'];
className?: string;
customDragHandle?: boolean;
spacing?: EuiDraggableSpacing;
Copy link
Owner

Choose a reason for hiding this comment

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

Niiiice

/**
* Adds an EuiPanel style to the droppable area
*/
withPanel?: boolean;
Copy link
Owner

Choose a reason for hiding this comment

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

I also considered doing this. Thanks!

@include euiBottomShadow;
border: 1px solid $euiDraggableFocusColor;
// Commenting this out for now,
// I'm thinking about adding an optional prop to auto-add these styles versus always having them
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@thompsongl
Copy link
Owner

Lastly, in the complex example, I changed the custom drag handle here to an EuiButtonIcon and it's not working as expected. Is it possible to allow this?

Are you looking to have the click toggle the dragging state, or just have the button be the drag handle, or both?
disableInteractiveElementBlocking prop can be added to that EuiDraggable for the second option, but we'd have to add functionality for the first option, which seems reasonable

@cchaos
Copy link
Author

cchaos commented Mar 22, 2019

just have the button be the drag handle

@cchaos
Copy link
Author

cchaos commented Mar 22, 2019

You can merge in now and hook up those couple buttons I added.

@cchaos
Copy link
Author

cchaos commented Mar 22, 2019

I'll work on the extra props later

@thompsongl thompsongl merged commit cdeeab6 into thompsongl:draggable Mar 22, 2019
thompsongl pushed a commit that referenced this pull request Aug 20, 2019
Update defazio fork to 13.0
thompsongl pushed a commit that referenced this pull request Feb 24, 2020
* removed duplicate icons

* Fixing changelog and updating tests (#1)

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants