-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Enable users to jump arbitrary page numbers from text field #739
Enable users to jump arbitrary page numbers from text field #739
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #739 +/- ##
==========================================
+ Coverage 68.20% 69.48% +1.27%
==========================================
Files 35 35
Lines 2334 2353 +19
==========================================
+ Hits 1592 1635 +43
+ Misses 742 718 -24 ☔ View full report in Codecov by Sentry. |
Could you review this PR? |
I will review it detail later, but can you add e2e test for loading journal storage file similar to sqlite here? optuna-dashboard/e2e_tests/test_standalone/test_study_list.py Lines 28 to 74 in 6b05c98
|
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.
page={page} | ||
labelDisplayedRows={({ page }) => | ||
`${page + 1} of ${maxPageNumber}` | ||
} |
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.
[nits] It seems that we should use default values here. I surveyed some templates for pagination and this one is similar to what you implemented. https://ui-patterns.com/patterns/Pagination/examples/691
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.
How about adding max page number somewhere around PaginationTextFieldComponent
?
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.
[Question] Does this mean I should simply remove labelDisplayedRows
and add maxPageNumber
to the text field?
I reflected what I assumed from your response, so could you confirm it from my modification?
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.
Sorry for my poor explanation. You understood what I meant and fixed them. Thank you.
) => { | ||
event.preventDefault() | ||
const newPage = parseInt(specifiedPageText, 10) | ||
setSpecifiedPageText("") // reset the input field |
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.
[nits] This line should be the last line of handleSubmitPageNumber
.
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 fixed it, but could you tell me why it would be better for my knowledge?
Is it just because of code preference or because of its code behavior?
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.
Sorry for my poor explanation. The setSpecifiedPageText("")
should be the procedure if the process is correctly handled. In that sense, this line should be put on the last line. More detail, the reason is that if we use early return e.g. if (Number.isNaN(newPage)) {
, we should keep text in the field because the user can fix it later.
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 for the clarification! Your explanation actually makes sense")
onPageChange={handleChangePage} | ||
onRowsPerPageChange={handleChangeRowsPerPage} | ||
/> | ||
{maxPageNumber > 4 ? ( |
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.
[question] Why did you choose 4 here?
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 will quickly leave a comment before I forget.
For the other comments, I will address then when I'm back to the work!
The reason is very simple: When we move from page X to Y by the text field, we need three actions. (1) Click the text field, (2) Type a number, (3) Hit the enter key.
Meanwhile when we would like to move any pages for 4-page data grid, we need to make less than or equal to three actions from A to B. Namely, we need to click the prev/next key less than or equal to three times.
That's why it's unnecessary (or at least, not more efficient ) to use the text field in this case.
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.
Is it better to let it stay always?
Plus, is there any other stuff to address?
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 understand it. But the reason is not so obvious if we do not put code comment there. We can choose the following options:
- Add comments for 4,
- Change the number from 4 to 2 (because 2 is much easier to understand the reason), or
- Always show it.
501dd2f
to
37fbdd6
Compare
@keisuke-umezawa |
So sorry, it is a wrong message. you can ignore it. |
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.
It looks good to me other than a nit. I will merge it after your change.
onPageChange={handleChangePage} | ||
onRowsPerPageChange={handleChangeRowsPerPage} | ||
/> | ||
{maxPageNumber > 4 ? ( |
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 understand it. But the reason is not so obvious if we do not put code comment there. We can choose the following options:
- Add comments for 4,
- Change the number from 4 to 2 (because 2 is much easier to understand the reason), or
- Always show it.
37fbdd6
to
5f73021
Compare
@keisuke-umezawa I reflected your suggestion |
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! Thank you for working on it.
Contributor License Agreement
This repository (
optuna-dashboard
) and Goptuna share common code.This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.
Reference Issues/PRs
This PR solves issue#504.
What does this implement/fix? Explain your changes.
This PR adds the following features:
After the Modification
Too many rows to click next many times.
We can specify the page number to jump.
The page number was successfully jumped to the specified page after hitting the enter key.