-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Sort table names alphabetically when creating new triggers #1194
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
|
Beep boop! 🤖 Hey @pthm, thanks for your PR! One of my human friends will review this PR and get back to you as soon as possible. 🕐 Stay awesome! 😎 |
|
Review app for commit 51d8c30 deployed to Heroku: https://hge-ci-pull-1194.herokuapp.com |
|
@pthm - Thanks for the PR. As you said, this alphabetical sorting can be moved into Can you make the required changes and update the PR? |
| > | ||
| <option value="">Select table</option> | ||
| {tableListBySchema.map(t => { | ||
| {tableListBySchema.sort((a, b) => { |
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.
Let's move the sort to the main function to reflect it in other parts of the app where its reused.
|
Review app for commit 23068f5 deployed to Heroku: https://hge-ci-pull-1194.herokuapp.com |
| columns: ['*'], | ||
| order_by: { | ||
| column: 'name', | ||
| order: 'asc', |
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.
The key is type and not order
|
@wawhal - Can you fix the conflict and the other issue mentioned above? |
|
Review app for commit 51bad5d deployed to Heroku: https://hge-ci-pull-1194.herokuapp.com |
|
Review app for commit e12789f deployed to Heroku: https://hge-ci-pull-1194.herokuapp.com |
|
Review app for commit 38bc06b deployed to Heroku: https://hge-ci-pull-1194.herokuapp.com |
karthikvt26
left a comment
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.
LGTM
|
Beep boop! 🤖 Whoa! 🎉 🎉 💃 Awesome work @pthm! 💪 🏆 All of us at Hasura ❤️ what you did. Thanks again 🤗 |
|
Review app https://hge-ci-pull-1194.herokuapp.com is deleted |
Description
What component does this PR affect?
Requires changes from other components? If yes, please mark the components:
Related Issue
No related issue
Solution and Design
Small change to sort table names alphabetically in the Schema/Table selection section of the add trigger screen. Currently its not sorted and appears in some arbitrary order as returned by the
fetchTableListBySchemafunction. This could be moved into that function to ensure its ordered in a usable way in other parts of the application if that's considered desirable.When dealing with hundreds of tables it becomes quite tiring searching through the unordered list for the table you want this is potentially made even worse depending on your table naming scheme.
Sorting the table names alleviates some of that strain, personally I think this could be improved further by using something like
react-selecton this select element.Type
Checklist: