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

Scientific Notation checkable when Forces Values is not checked #207

Closed
terracoda opened this issue Nov 20, 2019 · 37 comments
Closed

Scientific Notation checkable when Forces Values is not checked #207

terracoda opened this issue Nov 20, 2019 · 37 comments

Comments

@terracoda
Copy link
Contributor

I thought we solved this issue already, however, I can't find the original issue.
Issue: I can check Scientific Notation checkbox when Force Values is not checked. This should not be possible because no force values are showing.

I am pretty sure there is artwork demonstrating these the greyed-out sates.

  • Scientific Notation checkbox should be greyed-out AND disabled in the PDOM when Force Values is not checked.
  • And, if Scientific Notation is checked when Force Values gets unchecked, the Scientific Notation checkbox should get greyed-out and disabled in its checked state.

Screen Shot 2019-11-20 at 10 38 08 AM

@terracoda
Copy link
Contributor Author

@zepumph, there are SImple Context Responses in the design doc for these scenarios here in the design doc:
https://docs.google.com/document/d/1-37qAgde2XrlXBQae2SgjartM35_EnzDD9pdtd3nXAM/edit#

@terracoda
Copy link
Contributor Author

@zepumph, the responses are all correctly implemented, it is is just the fact that the Scientific Notation checkbox is enabled (checkable and uncheckable) when Force Values is not checked. It should be disabaled in when Force Values is unchecked.

I would try the HTML5 disabled attribute rather than the aria-disabled attribute
https://www.w3.org/TR/2014/REC-html5-20141028/disabled-elements.html

From https://caniuse.com/#search=disabled, the HTML5 attribute looks like a safe bet.

@zepumph
Copy link
Member

zepumph commented Nov 20, 2019

Moving to gravity force lab regular

@zepumph zepumph transferred this issue from phetsims/gravity-force-lab-basics Nov 20, 2019
@zepumph
Copy link
Member

zepumph commented Nov 20, 2019

@terracoda this was actually a purposeful design decision implemented for PhET-iO. The reasoning is because it makes for an easier api for customization. Can you outline the reasoning for wanting this for Interactive Descriptions. Perhaps we will need to rope in @arouinfar and @kathy-phet to this issue too for comment.

@terracoda
Copy link
Contributor Author

terracoda commented Nov 20, 2019

@zepumph, interesting about changing to simplify for PhET-iO. I understand the challenge.

However, @arouinfar and I had already discussed this at the beginning of the design process, and thougtht that we came up with a nice solution. I wish I had been looped in for the PhET-iO meeting ;-)

Our reasoning (from memory), essentially, it makes no sense to be able to turn on Scientific Notation if the Forces Values checkbox is not already checked. Scientific Notation is a second numerical representation of the Force Values. If Force Values are not shown (or described) then what does it mean to be able to turn on Scientific Notation - nothing. There is no numerical representation to convert to scientific notation - nothing happens visually when I check the Scientific Notation checkbox when Force Values is not checked.

The only thing that happens is that a context response fires saying "Forces now shown in newtons with Scientific Notation" - but that's not true!

@arouinfar, are you aware of this change?

@terracoda
Copy link
Contributor Author

terracoda commented Nov 25, 2019

@arouinfar, when you get a chance please share your thoughts. Let me know if you think this warrants some time in a design meeting soon.

@arouinfar
Copy link

Unfortunately, I think I was the only one present for both the PhET-iO and a11y conversations around this particular issue, and they were separated enough in time that I wasn't thinking about both. My apologies.

In general, I think disabling the checkbox is the cleaner option. However, we were willing to accept the slightly odd UX of not disabling the checkbox for the simplification of the api for PhET-iO, but this seems extremely problematic for the PDOM. I cannot remember if disabling the checkbox was slightly inconvenient/messy for PhET-iO or truly problematic in the same way the reverse is true for the PDOM. @zepumph can you comment on this (or point us to the appropriate issue)?

@arouinfar arouinfar assigned zepumph and unassigned arouinfar Dec 3, 2019
@kathy-phet
Copy link

I think it was problematic for PhET-iO - at least given the structure of the code at the time. I think it would be best to just schedule a short design meeting with @terracoda and @zepumph to see what options we can come up with for solving the puzzle.

@terracoda
Copy link
Contributor Author

It is also a relatively awkward and tricky issue for the PDOM, so thankful for the design time.

@zepumph
Copy link
Member

zepumph commented Dec 3, 2019

I too will be happy for design time.

To try to summarize, it seems challenging for the PDOM because we have an alert when the scientific checkbox is toggled that states that the force vectors are now displayed in a different way. This is confusing if there aren't even any force vectors displayed.

It was problematic for PhET-iO because the "enabledProperty" for the scientific notation checkbox was the Property that controlled the force values checkbox. Because of the way this was set up. There was no way to set the scientific notation checkbox to enabled:false through the PhET-iO api without the user just able to toggle it back to enabled by clicking on the force values checkbox again.


I think that the solution is to revert to the previous solution, where the scientific notation box toggles enabled, and add a Property only accessible via PhET-iO called scientificNotationCheckboxEnabledOverrideProperty or something like that, which can override the current enabled of the checkbox and will always be correct when set through the PhET-iO api. The reason why we didn't do this before was that it was added complexity that didn't seem warranted. It now definitely seems warranted to me. If I have time I can hack our a prototype to play around with before the design meeting.

@zepumph
Copy link
Member

zepumph commented Dec 5, 2019

Today in design meeting we thought about how these two checkboxes are actual representing 3 states. What if we used a radio button group instead?

  • "Show Values"
  • "Scientific Notation"
  • "Hidden"

This will solve PhET-iO because then we won't have dependent Property values. For

@arouinfar will do a mockup and get back to me for implementation.

@zepumph zepumph assigned arouinfar and unassigned terracoda, kathy-phet and zepumph Dec 5, 2019
@terracoda
Copy link
Contributor Author

@zepumph
Copy link
Member

zepumph commented Dec 5, 2019

Oops. I assigned @terracoda without seeing the above comment because the descriptions will also need work. Please unassign if you feel like they are ready.

@arouinfar
Copy link

Post-meeting slack conversation:

Taliesin Smith 3:10 PM
Sadly, in the design meeting I did not mention the help text that accompanies each checkbox. The help text is what creates the context for the checkboxes before interaction takes place and thent eh responses confirm the change in context. So ultimately, there is a shared responsibility of the words in the label, the checked/not checked state, the words in help text, and the words in the response (alert) once the control is toggled. I am sorry I did not communicate that fully in the meeting. I was unprepared for discussing a entirely new design - my fault. (edited)

That said, the radiobuttons are an interesting solution, but the overall experience is quite different. The radiobutton group will have one help text for the group. And again, I apologize for not making the differences more clear during the meeting - the differences in the interaction and the associated descriptions they entail.

Michael Kauzmann 3:16 PM
Could a single new help text for the radio button group be something like "Explore the force with different values."

Taliesin Smith 3:16 PM
The values are the same, the representations are different.

I mocked up a table in the design doc. There are 2 tables people can look at to compare here https://docs.google.com/document/d/1-37qAgde2XrlXBQae2SgjartM35_EnzDD9pdtd3nXAM/edit#heading=h.m011037v6ep4

The tables have all the descriptions so people may be able to see the full context which I did not explain. The radiobuttons could be a good solution, but I was just having a feeling like, "If it ain't broke, why fix it with radiobuttons?" And it is broken for phet-io, so maybe we do need to fix it with radiobuttons 🙂 No users had any trouble with the GFL checkboxes as they were designed.

Looking forward to comments.

Kathy Perkins 3:39 PM
It has an alternative "fix" for PhET-iO - so alternatively we could do that instead for this sim - and then avoid this type of design in the future.

@zepumph
Copy link
Member

zepumph commented Dec 31, 2019

The stroke around the Force Values panels looks a bit heavy, can you match the stroke weight used on the Mass panels?

What was happening here is that actually the mass controls are being scaled down. I changed it so none of the panel strokes were scaled, ergo I made them all heavy but consistent. This felt like a better change than trying to map it the other way. If you think it is too heavy, then we can change all of the line widths.

@ariel-phet suggested shifting the starting locations of the masses. Currently they are aligned to the 3 m & 7 m marks on the ruler. For better alignment with the Mass panels, let's shift everything left 1 m, with the starting positions at 2 m and 6 m.

@terracoda does this sound like an appropriate change in terms of interactive description content? I can't think of any issues with it.

@terracoda
Copy link
Contributor Author

@zepumph, thanks for asking.
I do not see any issues with it.

  • Force arrows and distance are still the same.
  • m1 will have a little less initial wiggle room, but that is the same for both visual and non-visual experiences.
  • m2 will be 2 steps closer to crossing to the left side of the track and m1 will be two steps farther from crossing to the right side of the track.

I don't think any of this has any pedagogical ramifications.

@terracoda terracoda removed their assignment Dec 31, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Dec 31, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Dec 31, 2019
zepumph added a commit that referenced this issue Dec 31, 2019
zepumph added a commit to phetsims/coulombs-law that referenced this issue Dec 31, 2019
zepumph added a commit to phetsims/gravity-force-lab-basics that referenced this issue Dec 31, 2019
zepumph added a commit to phetsims/gravity-force-lab-basics that referenced this issue Dec 31, 2019
@zepumph
Copy link
Member

zepumph commented Dec 31, 2019

I did a bunch more work here. Please review the following changes:

  • I implemented all of the checkboxes @arouinfar recommended
  • I made this same change to Coulombs Law, since it has the same issue.
  • I refactored the checkbox group into GFLB since BASICS is the only sim that uses it now
  • I cleaned up a lot of TODOs from yesterday.
  • I PhET-iO instrumented the new radio buttons much better, and they are ready for review now.
  • As part of the PhET-iO step, I removed the phetioFeatured overrides for the scientificNotationProperty (as it no longer exists), and the showForceValuesProperty, and added phetioFeatured for the new forceValuesDisplayProperty. Let me know what you think.
  • I DID NOT end up diving into the base logic for showValuesProperty, instead I think it is easiest to just keep it, since all three sims use that, and only two of the three sims have a forceValuesDisplayProperty. The result of this is simpler common code logic, and a duplicated link in CL and GFL to make sure that showForceValuesProperty is kept in sync as forceValuesDisplayProperty is changed via the UI.

Assigning to @kathy-phet and @arouinfar for review.

@zepumph zepumph assigned kathy-phet and arouinfar and unassigned zepumph Dec 31, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Dec 31, 2019
@arouinfar
Copy link

@zepumph Coulomb's Law is looking good, but the gap between the checkbox and "Constant Size" looks a bit large in GFL. Can you left-align "Constant Size" with the strings above it?

Things also look good in studio. The only thing that looks a bit odd to me is the constantRadiusCheckbox and constantRadiusProperty. Both GFL and GFLB use "Constant Size" for the string, so it's a bit strange to call it "Constant Radius".

@arouinfar arouinfar assigned zepumph and unassigned arouinfar Jan 3, 2020
zepumph added a commit that referenced this issue Jan 7, 2020
@zepumph
Copy link
Member

zepumph commented Jan 7, 2020

I decreased the spacing from 10 to 4, and I feel like things are even now

image

See #230 for tandem naming. @arouinfar are we ready to close here?

@zepumph zepumph assigned arouinfar and unassigned zepumph Jan 7, 2020
@arouinfar
Copy link

Looks great @zepumph!

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

5 participants