-
-
Notifications
You must be signed in to change notification settings - Fork 185
fix!: Place sort handle inside k-item boundaries
#7361
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
base: v6/develop
Are you sure you want to change the base?
Conversation
k-items
|
This is the kind of change where I wonder if we would actually need to call it "breaking". At least in terms of the markup and CSS for items. Unfortunately the items are so dominant in the panel that I can see how this could be an issue. |
|
I totally understand that point. At the same time I am not sure we can leave sorting pretty broken in the list layout for all of v5. |
|
I tried a couple more things to get this done via a CSS-only solution, but it all feels like a hack. I think your solution really is the most stable one. It's nitpicky, but maybe we can find a better name for the wrapper? Something like k-item-box maybe? |
|
I just saw a comment on the sortablejs repo that position absolute would work as long as the same z-index. Maybe we could try that. Otherwise, open to any name for the new div . |
|
Nevermind, that was also only true when the handle is placed absolute but inside the parent. So I think we really have to restructure it like in this PR. But not sure if in v5.x or v6. |
|
Can we maybe split the PR? All your non-destructive changes to the items component could be immediately merged and I think they already help to fix a few things. With the other parts, we might think again if there's a CSS-only solution for v5 and then a more drastic solution for v6? |
de9fa82 to
6087f7f
Compare
k-itemsk-item
6087f7f to
4b0cc8a
Compare
k-itemk-item boundaries
4b0cc8a to
01adefb
Compare
k-item boundariesk-item boundaries
2bfe119 to
656a835
Compare
Fixes #7349 BREAKING CHANGE: DOM structure of `k-item` has been changes. An additional wrapper `.k-item-box` div was added.
656a835 to
96ad748
Compare
cfa9f36 to
6ae111e
Compare
|
@distantnative I think you can resolve the conflicts better than me, but the PR definitely looks good. |
|
@bastianallgeier I will wait for the multi-select PR first, to not have to do it twice. |
Description
selecteditems & radio choice #7516This PR improves drag'n'drop sorting of
k-itemsfor list layout: Instead of positioning the sort handle absolute outside of thek-itemsboundaries, it is now positioned relatively inside the container. This is necessary as SortableJS gets tripped up by the absolute positioning outside. To achieve the same look/style as before, a new.k-item-wrapperdiv was added and the main styles applied to this.Changelog
Fixes
Problem changing order of page list items in panel #7349
Breaking changes
k-itemhas been changes. An additional wrapper.k-item-boxdiv was added.For review team