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 DialogBox.fxml to fix text overrun #44

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

JiaXinEu
Copy link
Contributor

@JiaXinEu JiaXinEu commented Jul 10, 2024

Change styling of DialogBox directly in the FXML file.
As discussed in #22, this is an alternative to PR #37

@damithc
Copy link
Contributor

damithc commented Jul 11, 2024

@flexibo is this what you meant?

@flexibo
Copy link
Contributor

flexibo commented Jul 11, 2024

@damithc Yes. I do see your point about teaching students to learn about the problem, maybe in addition to this change, we can explain what minHeight="-Infinity" does?

Maybe a side by side comparison?

@damithc
Copy link
Contributor

damithc commented Jul 20, 2024

@JiaXinEu perhaps we can still add an explanation about why we use this minHeight="-Infinity"

Copy link
Contributor

@laney0808 laney0808 left a comment

Choose a reason for hiding this comment

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

LGTM.

@JiaXinEu
Copy link
Contributor Author

perhaps we can still add an explanation about why we use this minHeight="-Infinity"

Updated with a note on the effect of setting minHeight = "-Infinity" and a comparison without setting minHeight @damithc @flexibo

flexibo

This comment was marked as duplicate.

Copy link
Contributor

@flexibo flexibo left a comment

Choose a reason for hiding this comment

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

LGTM.


![No minHeight for Label](images/javafx/OverrunDialogBox.png)

</box>
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can make it easy to spot the problem in the second screenshot?
e.g.,
image

Also, try to make the example screenshots not bigger than needed, so that they don't consume valuable screen space more than they need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the second screenshot and cropped them such that each screenshot only shows one complete DialogBox .

@damithc damithc merged commit 1729ebe into se-edu:master Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants