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

Distance Ranges: Add new ranges for described distance (changed title) #88

Closed
terracoda opened this issue Jan 21, 2019 · 13 comments
Closed

Comments

@terracoda
Copy link

I have already identified several places in PDOM where the distance value will be hidden when the Distance checkbox is not checked (@terracoda to add a list here)

I still need to work out, however, how the Distance checkbox will affect the aria-valuetext of the position/distance sliders.

I will work this out in the design doc and then update this issue. Mentioning you, @zepumph, as heads-up as you familiarize yourself with this sim.

@terracoda terracoda self-assigned this Jan 21, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Feb 13, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Feb 13, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Feb 13, 2019
@zepumph
Copy link
Member

zepumph commented Feb 13, 2019

In the above commits I hopefully added the qualitative value text alert changes when the distances are not shown. Please review.

@zepumph
Copy link
Member

zepumph commented Feb 13, 2019

Thoughts about current version that @terracoda and I discussed this evening morning:

  • On the last step when you move a mass so they are touching, you hear "Vector" instead of force arrow. Something like "Vectors very small, forces N newtons"
  • Farthest from and closest to value texts should be added. That can likely be tacked on to the other qualitative distance regions.
  • Update the range of relative distance table because in BASICS the min is 1.3 (km), but in REGULAR the min is .3 (m)
  • For now we will leave the "meter mark" piece of the position alerts when distance values aren't showing, even though it is technically it is a kilometer number, they are distance.

@terracoda
Copy link
Author

terracoda commented Feb 14, 2019

The qualitative distance sounds really good!

I identified 4 issues over in the GFL REGULAR repo which also exist in BASICS:

In addition there are 1 more issue that only exist in BASICS for the qualitative distance descriptions:
Region names are silenced, on the second or third step (see Steps 5, 13, and 15) if the step is still in the same region. I think this is how I asked MB to originally implement. Thus, this issue is not blocking for interviews. It sounds fine, but I think I would rather silence the region name on the first step/first change in direction when the user gets the progress indicator, “Closer” or “Farther away”. I think this is how it works in John Travoltage.

  • Two options to address this issue:
    • Do nothing for now, we wait until after interviews, and I warn interviewers.
    • If there is time, implement Opt 2 that is noted in Steps 1, 2, 3 and 11.

Again, great work @zepumph!

@terracoda
Copy link
Author

@zepumph, I verified aria-valuetext and alerts for both sims. I still need to update the ranges for the regions.

@terracoda
Copy link
Author

terracoda commented Feb 14, 2019

@zepumph, I have updated the ranges in Table Distance Regions Names in the BASICS design document.

While testing, I noticed we have an extra region name, "not so far from".

When you implement the new ranges can you remove "not so far from"?

@terracoda
Copy link
Author

terracoda commented Feb 14, 2019

Just adding a visual of the table in case it is helpful. I'll replace the screen shot if anything changes.
ranges-typo-corrected

Updated to correct a typo.

@terracoda terracoda removed their assignment Feb 14, 2019
@terracoda
Copy link
Author

@zepumph, top priority for interviews, please address the 2 remaining checkboxes in comment
#88 (comment)

  • change "Vectors" to "Force arrows" in the force alert when a sphere reaches the last stop.
  • add in the new single-value regions and adjust the region ranges according to the updated table

I checked off the third checkbox as I have added the new single-value regions, "Farthest from" and "Closest to" and updated the ranges in table, see previous comment.

For now, ignore comment #88 (comment). Do do nothing about the silent regions.

@terracoda terracoda changed the title Identify effects on descriptions and alerts when showing/hiding distance in this sim Add new ranges for described distance (changed title) Feb 19, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Feb 19, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Feb 19, 2019
zepumph added a commit to phetsims/inverse-square-law-common that referenced this issue Feb 19, 2019
@zepumph
Copy link
Member

zepumph commented Feb 19, 2019

The two checkboxes above have been implemented now.

@terracoda
Copy link
Author

Note, I posted a new issue for the first checkbox about "vectors".

@terracoda
Copy link
Author

terracoda commented Feb 25, 2019

@zepumph, this issue is somewhat related to issues #94, #100, #101 which toggle quantitative distance with qualitative distance descriptions in the PDOM.

I think it is best to implement the new ranges for the described qualitative distance, and then work on the how the quantitative and qualitative values toggle in the PDOM when the Distance checkbox is checked and unchecked which is covered in #94, #100, and #101 .

@terracoda terracoda changed the title Add new ranges for described distance (changed title) Distance Ranges: Add new ranges for described distance (changed title) Feb 25, 2019
@zepumph
Copy link
Member

zepumph commented Feb 26, 2019

@terracoda I'm a little confused, I thought that these had already been done. Is there something more you want here? Currently the ranges are implemented as follows (matching what I see currently in the design doc):

      if ( distance === 9.6 ) {
        return 0;
      }
      if ( distance >= 9.0 ) {
        return 1;
      }
      if ( distance >= 7.6 ) {
        return 2;
      }
      if ( distance >= 6.1 ) {
        return 3;
      }
      if ( distance >= 4.6 ) {
        return 4;
      }
      if ( distance >= 3.1 ) {
        return 5;
      }
      if ( distance >= 2.0 ) {
        return 6;
      }
      if ( distance >= 1.4 ) {
        return 7;
      }
      if ( distance <= 1.3 ) {
        return 8;
      }

with index values returned accessing these lists (0 indexed so 0 is the first item).

  const RELATIVE_DISTANCE_STRINGS = [
    farthestFromString,
    extremelyFarFromString,
    veryFarFromString,
    farFromString,
    notSoCloseToString,
    closeToString,
    veryCloseToString,
    extremelyCloseToString,
    closestToString
  ];

  const DISTANCE_STRINGS = [
    farthestString,
    extremelyFarString,
    veryFarString,
    farString,
    notSoCloseString,
    closeString,
    veryCloseString,
    extremelyCloseString,
    closestString
  ];

@terracoda
Copy link
Author

@zepumph, you are absolutely correct the new ranges are implemented, apologies. They are perfect in BASICS! A range has no more than 3 default key presses which I find super nice.

Could you clarify the difference between the 2 sets of distance strings above. Assuming they are literal representations, you need to use the full string including the preposition "to" or "from" in the scene summary.

Actually, I'll go ahead and close this issue and comment over in #100 where this information is more relevant!

@terracoda
Copy link
Author

The new ranges for distance are implemented beautifully, 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

2 participants