-
Notifications
You must be signed in to change notification settings - Fork 65
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
Replace Nestable with Sortable #3055
base: main
Are you sure you want to change the base?
Conversation
33c881f
to
56702a5
Compare
@@ -3,6 +3,7 @@ | |||
//= require sir-trevor | |||
//= require clipboard | |||
//= require tiny-slider | |||
//= require Sortable.min.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only used by the admin interfaces? Is that why the nestable import was in app/javascript/spotlight/admin/index.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe that's the case. I'll shuffle this back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to work well. I tested it on the Feature/About pages admin widget and the sir trevor blocks.
Great, thanks for testing. I think in terms of functionality it's nearly 1:1. There are styling differences on the ghost and the target destination, mostly around the current target being an outline only. Do you think that's a concern or are we willing to use Sortable's default styling there? I can get Alan's take on this if we need more input. Otherwise, I have a bit more testing specifically in the Sir Trevor blocks I'd like to do and maybe some clean-up. Then I'll propose this for real. |
@taylor-steve I'm not concerned about the styling changes, but does seem worth running by Alan. |
df4ee97
to
81d5144
Compare
Noting that nobody had concerns about the difference in styling. I do think it may be prudent to have one or two folks evaluate the ergonomics of nesting (e.g., feature pages). It has a slightly different feel than nestable, but I think it's quite close. Once we're ready to move to import maps we could pin Sortable and drop the local file if we wished. |
81d5144
to
7d7e178
Compare
7d7e178
to
0298800
Compare
Closes #2960.
This replaces the unmaintained Nestable library with Sortable. This is intended as a drop-in replacement, without modifying the existing template markup.
Currently Sortable is in vendor, matching our current behavior until we reorganize the JavaScript.
Sortable needs a little space between the last item of a first level nested list and the end of the containing parent list in order to match the behavior of Nestable (e.g., dragging the last nested item left into the parent list). That's why the padding has been added.
Might be overkill but this works for
data-max-depth
of 1-n, the old implementation was 1-2 (which is all Spotlight currently uses).