Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#5935 closed bug (notabug)

wrong comparison for center: >>... === verticalDefault << and >> ... === horizontalDefault << instead of >> ... === "center" <<

Reported by: T.Lindig Owned by:
Priority: minor Milestone:
Component: ui.position Version: 1.8.4
Keywords: Cc:
Blocked by: Blocking:

Description

in all places, where the offset option should normalize and checked if it should be centered, are wrong check like this:

if ( options.at[0] === "right" ) {
	basePosition.left += targetWidth;
} else if (options.at[0] === horizontalDefault ) {
	basePosition.left += targetWidth / 2;
}

But this is wrong code. The code only doing the right thing, because horizontalDefault is initialized with "center".

The correct code would be:

if ( options.at[0] === "right" ) {
	basePosition.left += targetWidth;
} else if (options.at[0] === "center" ) {
	basePosition.left += targetWidth / 2;
}

I found this wrong comparison for horizontalDefault and verticalDefault instead of "center" 4 times.

Change History (9)

comment:1 Changed 9 years ago by Scott González

Resolution: invalid
Status: newclosed

How is that wrong? They all have the same values.

comment:2 Changed 9 years ago by T.Lindig

Resolution: invalid
Status: closedreopened

It is wrong, because you don't want know if options.at[0] === horizontalDefault, you want know it if options.at[0] === "center".

horizontalDefault is a default value, how the name say it. It could be "left", "right" or "center". The check for that is at a other place.

in that place of code you want not know what the default-value is, you want know if you have to half the targetWidth. So you have to check for "center".

May be you should make a test, than you will see what i am talking about.

make horizontalDefault = "left"; and try to get a position centered trough the option at:"center". I bet with you, it will not work.

comment:3 Changed 9 years ago by Scott González

Resolution: invalid
Status: reopenedclosed

But horizontalDefault is not left, and never will be. This isn't user configurable. Would you feel better if I changed the variable name to center?

comment:4 Changed 9 years ago by T.Lindig

If horizontalDefault never be other than "center", for what is that var good for?

If you think it would be only to store the string "center", why did you not store the strings "right" and "bottom" too?

The var horizontalDefault and verticalDefault makes sense as default value. So it would be easy to change the default behavior and the code is better to read and understand. But in the 4 places i point out in my first message, it should be replaced by "center". because ' / 2' is what have be done ever for centering, regardless of defaultValue.

comment:5 Changed 9 years ago by T.Lindig

Resolution: invalid
Status: closedreopened

comment:6 Changed 9 years ago by Scott González

Resolution: invalid
Status: reopenedclosed

comment:7 Changed 9 years ago by T.Lindig

Resolution: invalid
Status: closedreopened

Scott, i don't understand your ignorance.

You should be grateful for help to improving the code quality of jquery ui. If you don't want help, you should not work in a open source project.

I show here a bug you can only find by code review and I show you a way to correct it. To see this bug you have to understand the code and what it does. The argumentations, that it "isn't user configurable" is very perfunctory and do not help the project.

Do not waste my time by closing this bug before it is fixed or disproved.

comment:8 Changed 9 years ago by Jörn Zaefferer

Resolution: invalid
Status: reopenedclosed

comment:9 Changed 7 years ago by Scott González

Milestone: TBD

Milestone TBD deleted

Note: See TracTickets for help on using tickets.