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

Calculation box changing size #6

Closed
KatieWoe opened this issue Oct 8, 2018 · 11 comments
Closed

Calculation box changing size #6

KatieWoe opened this issue Oct 8, 2018 · 11 comments
Assignees
Labels
type:question Further information is requested

Comments

@KatieWoe
Copy link
Contributor

KatieWoe commented Oct 8, 2018

The width of the partially hidden Area Model Calculation box (The middle option) changes when dragging the partitions. While the max width is changeable to allow for very large calculations, similar uses in the Area Model suite, or even the fully visible Area Model Calculation option, seem to have the width set at the width of the graph.
Found while working on phetsims/qa#201, but unrelated as it appears in Win 10 as well.
Steps to reproduce:

  1. Go to Area Model Decimals
  2. Go to the 2x2 or 3x3 scene
  3. Drag the area so it is larger than 1x1
  4. Select the partially hidden Area Model Calculation box
  5. Drag a partition back and forth, making sure it crosses a spot where one of the partitions = 1.

Example on partially hidden box:
twitching
Example on fully visible box:
notwitching

@KatieWoe KatieWoe added the type:question Further information is requested label Oct 8, 2018
@jonathanolson
Copy link
Contributor

Hmm, the reason this happens is because the line-by-line view mode requires extra arrow buttons (with currently extra padding):

area-expanded-box

This could be potentially solved by:

  1. Adjusting spacing/sizes so that this line will fit without expanding the box.
  2. It looks like the arrows are always either above or below where any calculation lines would be. So instead of shifting the arrows so they will be to the right of the calculation lines, instead they could just be above/below the calculation lines. Then no extra padding would be required for that mode.
  3. Expanding the "minimum" size of the box to include this extra size. It then wouldn't line up as nicely with the area above.

Thoughts on ideally 1 or 2?

@amanda-phet
Copy link
Contributor

I like either 2 or 3, so let's try 2 :)

So, in the cases that push the box to be wider, the box would instead stay the same width but the longest calculation line would simply run between the arrows a bit?

@jonathanolson
Copy link
Contributor

So, in the cases that push the box to be wider, the box would instead stay the same width but the longest calculation line would simply run between the arrows a bit?

Correct. Implemented above, so for example for decimals it doesn't cause a panel expansion:

screen shot 2018-11-06 at 11 16 10 am

but for cases like in the variables screen that require expansion anyways:

screen shot 2018-11-06 at 11 16 20 am

Does this look acceptable to you?

@amanda-phet
Copy link
Contributor

Eh, I've been sitting on this for a while and I think having the text spill into the scroll bar looks sloppy. Especially since - if I remember correctly- we were planning to treat that scroll bar as a "slider" for keyboard nav so you could easily press up/down instead of tabbing between two buttons.

I'd rather leave it as it is than make this change... but am still open to discussing it. I can't be conclusive at the moment but wanted to be sure to respond.

@amanda-phet
Copy link
Contributor

image

@jonathanolson
Copy link
Contributor

Understood! The change was committed to master, but I can easily revert if it's not desired.

@amanda-phet
Copy link
Contributor

@jonathanolson and I discussed this and decided to go with option 3. There are tradeoffs of course, but other considerations make it seem the most worthwhile.

jonathanolson added a commit to phetsims/area-model-common that referenced this issue Nov 12, 2018
jonathanolson added a commit to phetsims/area-model-common that referenced this issue Nov 12, 2018
@jonathanolson
Copy link
Contributor

Implemented. Can you check master to see if it looks good?

@KatieWoe
Copy link
Contributor Author

Looks good in my opinion. Just checked the situation described in the initial issue though.

@amanda-phet
Copy link
Contributor

Looks good to me. Thanks @jonathanolson

@jonathanolson
Copy link
Contributor

Sounds good, I'll ask about whether things are stable enough for branching at tomorrow's dev meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants