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

Fixes #37575 - Foreman Table columns sort is inconsistent #10213

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

MariaAga
Copy link
Member

To test you can open the models table in firefox and chrome and see that the order is different before the pr.
Firefox and Chrome implement the sort function differently

@ekohl
Copy link
Member

ekohl commented Jun 19, 2024

What if both weights are undefined? Should it sort on a secondary parameter like name or id?

@MariaAga
Copy link
Member Author

then it sorts it in the order the columns are in the object provided to the table

@ekohl
Copy link
Member

ekohl commented Jun 19, 2024

Shouldn't it still return 0 if both are undefined? Because then you can consider them equal.

@MariaAga
Copy link
Member Author

It will return 0 if both undefined since then the first if would be true columnBWeight === undefined

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

What I meant was that it needs to be:

if (columnAWeight === undefined && columnBWeight === undefined) {
        return 0;
}
if (columnBWeight === undefined) {
        return -1;
}
if (columnAWeight === undefined) {
        return 1;
}
return columnAWeight - columnBWeight;

Because the sort interface says that when it's 0 they should be considered equal. Your current change implies that if columnBWeight they're always equal, even if columnAWeight isn't undefined.

Another way to look at it is that the function should be its inverse with a swapped order, or sort(a, b) == -sort(b, a).

@MariaAga
Copy link
Member Author

Yeah changed to this logic instead, the weights are sorted in this way, from the doc:

  • weight - the weight of the column, which determines the order in which columns are displayed. Lower weights are displayed first.

@ekohl
Copy link
Member

ekohl commented Jun 20, 2024

@jeremylenz since I proposed the current version, would you mind reviewing?

Copy link
Contributor

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Tested and LGTM 👍

@jeremylenz jeremylenz merged commit 1bbdf51 into theforeman:develop Jun 20, 2024
66 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants