Skip to content

Fix table item list ordering #7366

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

Merged
merged 3 commits into from
Apr 3, 2025

Conversation

yoshiokatsuneo
Copy link
Contributor

What type of PR is this?

  • Bug Fix

Description

Ordering by clicking table column does not work well, as the sorting handled by toggling two status(on/off) on every click but actually the sorting have triple status ("asendant", "descendant", default).

This PR fixed the issue by using sorting status on Table component, instead of "guessing" the status from outside of component.

How is this tested?

  • Manually

Clicking table column changes the order as intended.

image

@yoshiokatsuneo yoshiokatsuneo force-pushed the fix/fix_item_list_sorting branch 6 times, most recently from fa84613 to 51a8351 Compare March 8, 2025 16:00
@yoshiokatsuneo yoshiokatsuneo force-pushed the fix/fix_item_list_sorting branch from 51a8351 to 2cb97a0 Compare March 8, 2025 16:08
@yoshiokatsuneo yoshiokatsuneo marked this pull request as ready for review March 8, 2025 16:49
@eradman
Copy link
Collaborator

eradman commented Apr 3, 2025

@yoshiokatsuneo what is the difference from the user's perspective? I tried a simple query, and 3 clicks on the "date" column (sort asc, sort desc, default) seems to work the same as before

SELECT 1 AS row, NULL AS date
UNION
SELECT 2, now() - interval '55 hours'
UNION
SELECT 3, now() - interval '1 day' 

@yoshiokatsuneo
Copy link
Contributor Author

yoshiokatsuneo commented Apr 3, 2025

@yoshiokatsuneo what is the difference from the user's perspective? I tried a simple query, and 3 clicks on the "date" column (sort asc, sort desc, default) seems to work the same as before

SELECT 1 AS row, NULL AS date
UNION
SELECT 2, now() - interval '55 hours'
UNION
SELECT 3, now() - interval '1 day' 

@eradman

The problem is not about sorting rows on each query's result table, but about sorting queries on query list page like below.

420573842-6dd6a7ea-2533-46b3-95f3-41612d442a09

@eradman
Copy link
Collaborator

eradman commented Apr 3, 2025

The problem is not about sorting rows on each query's result table, but about sorting queries on query list page like below.

Okay, but I still don't see incorrect results. Does this fix an error in sorting, or is this an internal code cleanup?

@yoshiokatsuneo
Copy link
Contributor Author

yoshiokatsuneo commented Apr 3, 2025

The problem is not about sorting rows on each query's result table, but about sorting queries on query list page like below.

Okay, but I still don't see incorrect results. Does this fix an error in sorting, or is this an internal code cleanup?

@eradman

This PR fix an error in sorting.
On each clicking "Created At", sorting order and sorting triangle mark changes like following sequence.

  • descending order, no triangle mark
  • ascending order, up triangle mark(▲)
  • descending order, down triangle mark(▼)
  • ascending order, no triangle mark
  • descending order, up triangle mark(▲)
  • ascending order, down triangle mark(▼)
  • descending order, no triangle mark
    ...

So, "sorting order" have 2-click cycle, but "triangle mark" have 3-click cycle.

@eradman
Copy link
Collaborator

eradman commented Apr 3, 2025

Aha! Tricky. Please add these bullet points to the commit message

@eradman
Copy link
Collaborator

eradman commented Apr 3, 2025

Aha! Tricky. Please add these bullet points to the commit message

Never mind, I can update the commit message when merging

@eradman eradman enabled auto-merge (squash) April 3, 2025 16:21
@eradman eradman merged commit 84262fe into getredash:master Apr 3, 2025
11 checks passed
@yoshiokatsuneo
Copy link
Contributor Author

@eradman Thanks a lot !

@zachliu
Copy link
Contributor

zachliu commented Apr 29, 2025

@yoshiokatsuneo thank you! i think i can finally create a PR for this #7117

@yoshiokatsuneo
Copy link
Contributor Author

yoshiokatsuneo commented Apr 29, 2025

@zachliu I think this PR was already merged, and the issue was already solved.

@zachliu
Copy link
Contributor

zachliu commented Apr 30, 2025

@yoshiokatsuneo Sorry for the confusion—you misunderstood my earlier message. What I meant was that I had intended to make the Created By column sortable as well. Now that you've fixed the ordering issue, I can create a separate PR for that.

@yoshiokatsuneo
Copy link
Contributor Author

@yoshiokatsuneo Sorry for the confusion—you misunderstood my earlier message. What I meant was that I had intended to make the Created By column sortable as well. Now that you've fixed the ordering issue, I can create a separate PR for that.

@zachliu
Ah, I see. Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants