-
Notifications
You must be signed in to change notification settings - Fork 34
Add table interactivity #442
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
Conversation
- Add search - Add sorting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awesome! Especially the search should come be pretty useful. I also tested that the table renders fine without JavaScript, which is also important.
Left one question and one nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
} | ||
fn get_queue_status_priority(status: &QueueStatus) -> u32 { | ||
match status { | ||
QueueStatus::ReadyForMerge(_, _) => 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing this function, I wonder if we should add the mergeability status into QueueStatus
, and have something like QueueStatus::Unmergeable
. Not sure if that would help with anything though, and definitely not for this PR anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wouldn't be able to use that variant in the merge queue itself since it filters out un-mergeable PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not really different from NotApproved
or Stalled
, which should already never be returned from the SQL query, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I guess there's no use for that variant (at least now).
This PR adds:
I chose to use DataTable as the table library since it had the easiest setup and I found it the most ergonomic for our use case.
Currently the design looks poor as I plan to do the design as the last step - functionality first.