Skip to content
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

Update Box in TrialListDetail scrollable #456

Merged
merged 2 commits into from
May 17, 2023
Merged

Update Box in TrialListDetail scrollable #456

merged 2 commits into from
May 17, 2023

Conversation

hrntsm
Copy link
Collaborator

@hrntsm hrntsm commented May 10, 2023

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.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

Fix #448

What does this implement/fix? Explain your changes.

The content mentioned in the above issue has been modified as shown in the following image.
In order to enable horizontal scrolling, maxWidth is set to 1000px for now, but the value will be modified if wider is better, etc.
The maximum height is set to 150px, but I just made sure that about 6 lines are displayed, and will modify this one if necessary.

If "alignItems" is set to "center", values above the center will not be displayed when vertical scrolling is enabled. Therefore, it is removed.
The result is that each value is not centered but slightly above the center.

Screenshot 2023-05-10 at 21 21 29

@c-bata c-bata self-assigned this May 12, 2023
Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hrntsm Thank you for your pull request! Looks almost good to me. I left one comment though.

@@ -226,10 +226,13 @@ const TrialListDetail: FC<{
? "rgba(255, 255, 255, 0.05)"
: "rgba(0, 0, 0, 0.05)",
width: "100%",
maxWidth: "1000px",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about removing this for wide display users?

Suggested change
maxWidth: "1000px",

Copy link
Collaborator Author

@hrntsm hrntsm May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@c-bata Thanks comment!

Without a maximum width setting, the window will be as wide as the length of the string.
I'm trying to fix a problem with a Human-in-the-loop Slider widget where the width of the slider is lengthened to match the length of the string.(see Horizontal scrolling part of the issue)
So I would like to remove the maximum width in the section you commented on, and add a maximum value to the slider width.
what do you think?

Copy link
Collaborator Author

@hrntsm hrntsm May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is intended to add maxWidth:"1000px" to the following.
https://github.com/optuna/optuna-dashboard/blob/main/optuna_dashboard/ts/components/TrialFormWidgets.tsx#L141

The following modifications are intended
Screenshot 2023-05-16 at 20 13 23

Copy link
Member

@c-bata c-bata May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would like to remove the maximum width in the section you commented on, and add a maximum value to the slider width. what do you think?

@hrntsm Thank you for your explanation. Sounds reasonable!

@@ -385,6 +386,7 @@ const ReadonlyFormWidgets: FC<{
marginBottom: theme.spacing(2),
margin: theme.spacing(0, 1, 1, 0),
p: theme.spacing(1),
maxWidth: "1000px",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@c-bata
I added a maxWidth setting so that it is the same size for Readonly widgets that have completed their evaluation, not just Updatable ones that have an evaluation in them.

Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! LGTM.

@c-bata c-bata merged commit dc4a11c into optuna:main May 17, 2023
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.

Can Attribute and Parameter fields be made scrollable?
2 participants