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

Short max widths for game answers #117

Closed
KatieWoe opened this issue Feb 4, 2019 · 22 comments
Closed

Short max widths for game answers #117

KatieWoe opened this issue Feb 4, 2019 · 22 comments
Assignees

Comments

@KatieWoe
Copy link
Contributor

KatieWoe commented Feb 4, 2019

For phetsims/qa#277 and phetsims/qa#278
When the correct answer is given on a game level and it says "your line" (the one written in black) in the bottom box, the answer string is very short. This can be noticeable without string Tests if the string is long enough. easiest to see with stringTest=long.
smallmaxwidths
yourlinebottom

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 5, 2019

EquationBoxNode is used for both the guess and answer boxes, where the equation's maxWidth is set like so:

60 equationNode.maxWidth = boxSize.width - ( 2 * X_MARGIN ); // constrain width for i18n

So I can't immediately speculate on why the answer equation would be smaller.

@pixelzoom
Copy link
Contributor

Cleanup and fix in the above commits.

This was a problem with 'Graph The Line' type challenge, as identified in the upper-left corner of the screen when running with ?showAnwers. E.g.:

1

It now behaves properly with stringTest=long, e.g.:

screenshot_1028

A subclass of 'Graph The Line' challenge is 'Place The Point' challenge. If the placement of the 3 points doesn't make a line, you'll see something like this:

2

For reasons that I won't try to explain... With stringTest=long, the '... is not a line.' text will not fill the full width of the box, like this:

screenshot_1027

In practice, this will not be an issue, so I'm not going to go through the gyrations required to make it the full width of the box with stringTest=long.

@KatieWoe please verify in master. If it looks OK, leave the issue open for regression testing in the next RC.

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Feb 5, 2019

Looks good on master

@pixelzoom
Copy link
Contributor

Pending regression testing in 1.3.0-rc.2.

@KatieWoe
Copy link
Contributor Author

There still seems to be a problem when the slope is undefined (seen in Graphing Lines level 1).
slopeundefined

@pixelzoom
Copy link
Contributor

Thanks @KatieWoe. Fixed in master and 1.3 in the above commits. Please verify in master. If all looks well, leaves this issue open for regression testing in 1.3.0-rc.3.

@KatieWoe
Copy link
Contributor Author

Doesn't happen in the first screens, but still happens on game screens. @pixelzoom

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 18, 2019

@KatieWoe can you please elaborate? What problem are you still seeing related to this issue? Below are screenshots for all of the cases that you've identified so far, and text in the bottom box is the full width of the box in all cases.

Screenshots of master with ?ea&stringTest=long&showAnswers.

Similar to screenshot in #117 (comment):

screenshot_1071

Similar to screenshot in #117 (comment), with undefined slope:

screenshot_1070

Similar to screenshot in #117 (comment), but with defined slope:

screenshot_1069

@pixelzoom pixelzoom removed their assignment Feb 18, 2019
@KatieWoe
Copy link
Contributor Author

Seen here:
whatimeangame

  1. stringTest=double
  2. Find a challenge where you control the slope
  3. Get problem wrong and observe the length of the equation in the Your Line box.
  4. Swing the line around as you would in the other screens
  5. Observe changed size

@pixelzoom
Copy link
Contributor

OK thanks. This is a new problem, created by the fix for the reported problem.

pixelzoom added a commit that referenced this issue Feb 18, 2019
@pixelzoom
Copy link
Contributor

@KatieWoe The problem reported in #117 (comment) is fixed, please verify in master. Also verify that this doesn't break #114.

@pixelzoom pixelzoom removed their assignment Feb 18, 2019
@KatieWoe
Copy link
Contributor Author

All looks good from what I can see

pixelzoom added a commit that referenced this issue Feb 18, 2019
@pixelzoom
Copy link
Contributor

Pending regression testing in 1.3.0-rc.4.

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Feb 19, 2019

I did have a quick question for GL though. I noticed that "not a line" change size depending on what happened before it. I think this was known (#117 (comment)) but I wanted to make sure before I oked the issue.
differentsizes
Will also be seen with stringTest=long

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 19, 2019

So here's the dilemma... For "Place the Point" challenges, there are 3 things that can be displayed in the bottom box (the "guess box"):

  • an equation (e.g. y = 2x + 3)
  • "x = N (undefined slope)"
  • "... is not a line"

Only one of these is displayed depending on where the 3 points are placed, and the other 2 are invisible. All 3 share a common parent (the box's content), and maxWidth is set on that common parent. But whether they are visible or not, all 3 contribute to the bounds of the content.

The problem only occurs when transitioning from "x = N (undefined slope)" to "... is not a line" because the former is a longer string. When transitioning from an equation to "... is not a line", there's no problem. This recording demonstrates that:

kapture 2019-02-19 at 10 44 51

A few things to consider:

  • This is unlikely to occur in practice (with typical strings).
  • If it does occur, there's no significant affect on usability.
  • Fixing this is likely to take several more hours and require another RC.

So my recommendation is that we do not address this for the current release, or possibly at all. @amanda-phet are you OK with that?

@amanda-phet
Copy link
Contributor

I agree with @pixelzoom that we should not address this. I'm not sure it is worth it ever, because of the considerations you listed (unlikely to occur, and no effect on usability).

@pixelzoom
Copy link
Contributor

Thanks @amanda-phet.

@KatieWoe anything else to do here, or OK to close?

@pixelzoom pixelzoom assigned KatieWoe and unassigned pixelzoom Feb 19, 2019
@KatieWoe
Copy link
Contributor Author

Should be ok to close. Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants