Navigation Menu

Skip to content

Commit

Permalink
Sortable: modified the contents of placeholder to a single " ". …
Browse files Browse the repository at this point in the history 
…Fixed #8135 - sortable: Horizontal sortable shifts causes elements to shift down.
  • Loading branch information
@bertterheide @scottgonzalez
bertterheide authored and scottgonzalez committed Feb 24, 2012
1 parent 58a5e23 commit b6e1f25
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion ui/jquery.ui.sortable.js
View file
Expand Up @@ -660,7 +660,7 @@ $.widget("ui.sortable", $.ui.mouse, {

var el = $(document.createElement(self.currentItem[0].nodeName))
.addClass(className || self.currentItem[0].className+" ui-sortable-placeholder")
.removeClass("ui-sortable-helper")[0];
.removeClass("ui-sortable-helper").html(" ")[0];

if(!className)
el.style.visibility = "hidden";
Expand Down

9 comments on commit b6e1f25

@shuckster
Copy link

Choose a reason for hiding this comment

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

The .html() call clobbers the width/height on the placeholder for both my horizontal and vertical sortables.

To fix it without reverting the nbsp insertion I had to add these lines underneath:

            .removeClass("ui-sortable-helper").html(" ")[0];

        el.style.width = self.currentItem[0].style.width;
        el.style.height = self.currentItem[0].style.height;

@bertterheide
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I am using both and I haven't had any of these problems. Are you sure it's not a CSS fault?

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

@shuckster I can confirm that this is a regression. It can be seen in our sortable tabs demo.

@bertterheide
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that i defined the placeholder in my CSS, so in my case it does work.

@shuckster
Copy link

Choose a reason for hiding this comment

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

@scottgonzalez Yes, I'm seeing the same thing.

@bertjh Nothing special in the CSS. There is some colour and padding information that's pulled in from a class-tag, and the width/height information using the style-tag. (There's also a float-left for the horizontal lists, of course). I do have a "position: relative;" set on each list-item, but I've just tested it and it makes no difference.

EDIT: Just saw your new comment. I can indeed set a stylesheet for "ui-sortable-placeholder" that fixes the problem, but I have list-items that can vary in size during the lifetime of the app so that's not a flexible solution for me.

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

@bertjh I'm inclined to revert this change since it caused a regression and your use case has an easy workaround. Do you have any ideas for how to fix the regression and your use case at the same time?

@bertterheide
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the problem by changing the "_update" function to

// 1. If a className is set as 'placeholder option, we don't force sizes - the class is responsible for that
// 2. The option 'forcePlaceholderSize can be enabled to force it even if a class name is specified
if(className && !o.forcePlaceholderSize) return;

//If the element doesn't have a actual height by itself (without styles coming from a stylesheet), it receives the inline height from the dragged item
p.height(self.currentItem.height()).width(self.currentItem.width())

at line 680. Can you confirm?

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that fixes it. I had thought the same thing originally, but was afraid that the checks were there for performance reasons. Digging into the code some more, it seems like the calls to update() are in appropriate places and this should be fine. Someone actually just filed a bug about this a few minutes ago :-P.

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

I landed the fix in 4f19289 and marked @bertjh as the author.

Please sign in to comment.