-
Notifications
You must be signed in to change notification settings - Fork 288
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
[AMORO-3139] Improve list content of optimizing page #3201
Conversation
b676cf2
to
e0ddc06
Compare
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.
Hi, chouchouji
Thanks for your contribution.
I left some comments, and I have spiltted the original issue into more subtasks so that front and backend could better follow up.
I suggest you make this PR clean to this subtask: #3234
And of course, the work you have done will not be wasted, we shall push sub-sequencial issues to catch up.
export function getOptimizerTableList( | ||
params: { | ||
optimizerGroup: string | ||
dbSearchInput: string | ||
tableSearchInput: string | ||
page: number | ||
pageSize: number | ||
action: string[] | ||
sortField: string |
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.
I think we shall need sortField as well as sortOrder for ordering feature.
Anyway Let's postpone this sort feature for now(preserve action) and keep this PR as clean as possible.
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.
Do you mean I should keep action and remove sortField?
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.
yes, or you could add a sortOrder to mark 'asend' or 'descend'?
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.
If we don't need sortOrder now, I will delete it.
The backend work is: #3236 |
d6c7db7
to
6ac01b1
Compare
@majin1102 Thanks for your review! I had updated code. Please check it when you are free. |
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.
Thanks for feeding back
I left one comment, PTAL
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
Why are the changes needed?
Link issue #3139.
Brief change log
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation