se-edu / guides

Style guides and tutorials for SE student projects
MIT License
5 stars 20 forks source link

[JavaFX-Part 4] Update FXML of DialogBox #22

Open JiaXinEu opened 2 weeks ago

JiaXinEu commented 2 weeks ago

Current:

image

Problem:

Text overrun may occur, and longer text will not be fully displayed but replaced with ellipsis instead.

Proposed:

Replace line with:

<Label fx:id="dialog" text="Label" wrapText="true" minHeight="-Infinity">
damithc commented 1 week ago

@flexibo any thoughts on this suggestion? I remember this aspect came up during the writing of part 5.

flexibo commented 1 week ago

@JiaXinEu @damithc ah yes, IIRC the issue is also fixed by removing the prefHeight for VBox. Either way works, will be great to update it in part 4.

damithc commented 1 week ago

@flexibo Thanks for the inputs. @JiaXinEu You can send a PR for this. It may be better to keep the original, point out the problem, and provide the two possible solutions.

JiaXinEu commented 1 week ago

Ok, submitted PR #37

flexibo commented 1 week ago

@damithc @JiaXinEu Actually I was wondering if it is better to just replace that line altogether using either one of the solutions (can just use JiaXinEu's solution). It might be a lot clearer for the students this way and avoid any confusion.

damithc commented 6 days ago

It might be a lot clearer for the students this way and avoid any confusion.

I'm fine with that too. Normally I prefer for students to learn about a problem and also how to solve it (rather than not encounter it at all) but this specific problem is not that important, so OK to avoid it entirely.

JiaXinEu commented 6 days ago

I have sent a PR based on this. #37

It may be better to keep the original, point out the problem, and provide the two possible solutions.

Perhaps I can create another PR for this so we can decide on which is better?

replace that line altogether using either one of the solutions

damithc commented 6 days ago

Perhaps I can create another PR for this so we can decide on which is better?

Sure @JiaXinEu