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

Added drag functionality to update precendence for sort condition #3316

Conversation

dikwickley
Copy link

Fixes #2349

Technical details

I have implemented the drag functionality from scratch without using any third party libraries.
It basically works by creating a ghost element that is draggable. While it is being dragged, it's position is used to calculate what index it is on and that updates the underlying list simultaneously.
When the pointer is released that list is updated to the original sort store.
The slide animation is achieved by https://svelte.dev/docs/svelte-animate#flip

@seancolsen please review.

Screenshots

Screen.Recording.2023-11-20.at.11.46.09.PM.mov

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@seancolsen seancolsen self-assigned this Nov 20, 2023
@seancolsen seancolsen added the pr-status: review A PR awaiting review label Nov 20, 2023
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

I've given this a very quick, preliminary review. This is really impressive work, @dikwickley!

I've tested this locally. It works perfectly.

The code needs some clean up:

  • Please fix the linting errors.
  • Fix the icons so that they're cleaner. See the code in that same file that looks like <Icon {...iconAddNew} />. Make your icon code work like that one. We shouldn't see <svg> inside a component unless it's a very low-level component.
  • 100 extra lines in this one component just to handle the dragging isn't great. I'm not opposed to merging it like this. But ideally, we'd abstract that code out into its own component so that the concerns are better separated and components can be more easily reused. See that comment you have, "Functionality for making sort entries draggable"... A comment like that is a good sign that the code needs to be pulled out into a new abstraction. The comment is useful, but this is the sort of code that can get real messy over time. The comment applies to the next 100 or so lines, but comments have no way to communicate that sort of scope. Other abstractions like functions, modules, components, and actions are better able to wrap up big chunks of code like this into nice tidy boxes so that they have descriptive names around all the code at once and clear interfaces with the world outside that code. If you want to take a crack at pulling this code out into some other abstraction, I think that would be cool. But it's not strictly necessary to merge this PR. I also wouldn't be opposed to using a 3rd party library for this, especially if there is one that plays well with Svelte. You might even be able to look at some of the drag and drop libraries to see how they structure their abstractions.

I'd like to review the code more thoroughly, but I'm heading off for a 1 week vacation soon so I'm short on time. I'll get back to this PR when I return. We'll definitely merge this. We just need to get it cleaned up a bit. Thanks for your hard work thus far!

Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

@dikwickley

Thanks for fixing the linting errors and cleaning up the icons. Perfect!

I said:

ideally, we'd abstract that code out into its own component so that the concerns are better separated and components can be more easily reused

I see that you moved a bunch of code into a DraggableSortEntries.svelte component. That's not exactly what I had in mind. What I'm after here is a general-purpose abstraction that handles drag-and-drop sorting without concerning itself with any Mathesar-specific functionality. Your new component just moves code around with really separating the parts that I'd like to be separated.

I wanted to take a crack at this, because I imagine there being other areas of Mathesar that would benefit from this same sort of drag-and-drop sorting functionality. For example, with the proper abstraction, implementing the same precedence updating feature within the Grouping dialog would be trivially easy.

I thought I might be able to easily pull your code out into a new component, but I'm embarrassed to admit that I kind of got nerd sniped after getting my head into this problem! I ended up getting carried away and spending way too much time on this! But it was a fun problem. I've pushed my code up to your branch.

I've made the following changes to the drag-and-drop sorting functionality:

  • I abstracted it all out into a set of three general-purpose Svelte actions. This means the changes within Mathesar-specific components are now very minimal.
  • I changed the event handling logic to use pointerCapture, as I mentioned in the original ticket. This simplifies some of the code because you can listen to events on the trigger itself instead of the body.
  • I changed the behavior when sorting so that no DOM mutations are made and all live sorting is done purely via CSS. This way we don't need to worry about creating and destroying the preview item and also hiding the grabbed item. This is a bit simpler and should be more robust in other cases where creating and destroying sort entry UI might have weird side effects.
  • I made some changes to better support touch devices and tested them on Firefox and Chromium on a touch laptop.

Additionally, I fixed a bug introduced by allowing the user to edit the column within the sort entries. Prior to my fix, editing a column would bump that entry to have the lowest precedence.

Would you be up for reviewing my changes, testing them out, and making sure they make sense?

If this looks good to you, feel free to merge.

@seancolsen
Copy link
Contributor

Oh wait: you won't be able to merge it yourself. You can submit a review or just post a comment, and I'll merge.

@dikwickley
Copy link
Author

Thanks @seancolsen, i'll take a look.

@dikwickley
Copy link
Author

@seancolsen this is really amazing. You have abstracted it out in a much functional way. I did try to abstract it out myself but failed as the code was quite coupled.
I have tested it out for all the use cases and it works flawlessly. I think it's good to go 🚀

@seancolsen
Copy link
Contributor

Thanks!

@seancolsen seancolsen added this pull request to the merge queue Dec 10, 2023
Merged via the queue into mathesar-foundation:develop with commit d375575 Dec 10, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Allow dragging to modify precedence of sorting conditions
2 participants