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

Adds horizontal drag. #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

alyjak
Copy link

@alyjak alyjak commented Nov 15, 2024

  • Modifies the API slightly to support the new feature

  • Enable multiple separate draggable containers within the same page using leptos::context::Provider

  • Updated the example to demonstrate column-wise and row-wise draggables

- Modifies the API slightly to support the new feature

- Enable multiple separate draggable containers within the same page using leptos::context::Provider

- Updated the example to demonstrate column-wise and row-wise draggables
Comment on lines 340 to +342
pub fn provide_drag_reorder<const COLUMNS: usize, E>(
panel_order: [RwSignal<Vec<Oco<'static, str>>>; COLUMNS],
) -> [NodeRef<E>; COLUMNS]
) -> ([NodeRef<E>; COLUMNS], DragReorderContext)
Copy link
Owner

Choose a reason for hiding this comment

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

Returning the DragReorderContext is a nice refactor, giving the user more control over when they actually want to provide it as a context.

The name provide_drag_reorder becomes a little misleading though, implying it provides the context. Maybe we can refactor this to be DragReorderContext::new? Similar to leptos' RwSignal::new, etc.

@@ -45,6 +45,7 @@ pub enum HoverPosition {
/// Registers a panel with drag reordering for a given ID.
pub fn use_drag_reorder<E>(
id: impl Into<Oco<'static, str>>,
transpose: bool,
Copy link
Owner

Choose a reason for hiding this comment

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

Should transpose be on use_drag_reorder? Or on provide_drag_reorder? It seems like the transposing should be done at the provide level?

@alyjak
Copy link
Author

alyjak commented Nov 18, 2024

I think you are right with your comments, but we should go further. How about I try refactoring such that this whole thing becomes a component? I'm thinking of an API something like the following. If this works, maybe it would be worth trying to get it integrated into thaw-ui or another component library?

pub enum ReorderableDirection {
    Horizontal,
    Vertical
}

#[component]
pub fn ReorderablePanels(
    panel_order: RwSignal<RwSignal<Oco<'static, str>>>,
    orientation: ReorderableDirection,
    panel_view: Fn(Oco<`static, str>) -> impl IntoView,
    #[param(Optional)] container_class: maybe_string,
) -> impl IntoView

With an API like this, the context can stay private, which would be good for ergonomics, while letting users control the panel contents as well as have access to the id -> order mapping. One thing I want to do is make one of my data structure's orders reflect the current ordering of the panels, which I would still be able to do with an api like this through creating an Effect or similar off of the panel order argument.

By providing container_class, users can style the container <div>, while the component can privately manage the node_ref. with panel_view being a callback that takes the panel id and provides a view, the user is in charge of the panel contents, and the component wraps those contents in a div that provides the draggable behavior.

@tqwewe
Copy link
Owner

tqwewe commented Nov 18, 2024

This could definitely work! I like all the ideas you've suggested and it sounds like a great direction to go in for this I think.

container_class gives the ability for users to style the column/row containers, however it does prevent more advanced things such as on click for containers, etc. I wonder if along with panel_view, there could be an optional container_view: Fn(Children) -> impl IntoView, or Fn(Vec<Panel>) -> impl IntoView, or something similar giving full control over each column/row container for advanced use cases.

If you're happy to take the time to try implementing something like this, I think its definitely a better approach to the raw hooks!

@alyjak
Copy link
Author

alyjak commented Nov 18, 2024

Good suggestion. I'm going to take a crack at this, I plan on using draggable lists all over my app, so I think this could be useful for the community. Wish me luck!

@tqwewe
Copy link
Owner

tqwewe commented Nov 18, 2024

Awesome! It's also worth noting I recently pushed a commit (b3a5b64) to main moving the calculated mouse position by the offset of where it clicked on the element from. So instead of dictating the position of the dragged element by the mouse position directly, its dictated by the center of the dragged element.

I'm not sure if this is an improvement or not, but just wanted to make you aware.

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