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

change look of disabled Slider #478

Closed
pixelzoom opened this issue Feb 28, 2019 · 11 comments
Closed

change look of disabled Slider #478

pixelzoom opened this issue Feb 28, 2019 · 11 comments

Comments

@pixelzoom
Copy link
Contributor

In phetsims/graphing-quadratics#112 and https://docs.google.com/document/d/1OfHhIDuaBt-hhXVtpnJgM3Wm0iByJ7dmXd-2UkphUR4/edit#, we concluded that Slider's disabled "look" is not desirable. The entire slider should gray out by changing opacity, like other UI components (e.g. Checkbox).

Assigning to @ariel-phet to prioritize and assign.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 1, 2019

Here's what a disabled Slider currently looks like in Graphing Quadratics (a & b sliders). The only visual change is the color of the thumb, and the default is thumbFillDisabled: '#F0F0F0'. This is inconsistent with disabled look of other UI components.

screenshot_1097

Here's what is desired by the Graphing Quadratics design team. The entire slider component has reduced opacity to indicate that it's disabled. This is consistent with other UI components.

screenshot_1096

@ariel-phet This is a relatively easy change, but I'm unclear on who the stakeholders are. Can you please run this change by whoever needs to sign off?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 1, 2019

Notes to self about changes to Slider that will achieve the desired look, as shown in #478 (comment):

SliderThumb:

  • delete param enabledProperty
  • delete option fillDisabled
  • rename option fillEnabled to fill

SliderTrack:

  • delete option enabledProperty
  • keep fillEnabled and fillDisabled, they are related to disabling a range of the track, not disabling the entire track

SunConstants:

  • add DISABLED_OPACITY: 0.3, so we're not duplicating this value in yet-another place

Slider:

  • delete option thumbFillDisabled
  • rename option thumbFillEnabled to thumbFill
  • add option disabledOpacity: SunConstants. DISABLED_OPACITY
  • change enabledObserver to:
    var enabledObserver = function( enabled ) {
      self.interruptSubtreeInput();
      self.pickable = enabled;
      self.cursor = enabled ? options.cursor : 'default';
      self.opacity = enabled ? 1 : options.disabledOpacity;
    };

@pixelzoom
Copy link
Contributor Author

I believe that @arouinfar was the only sim designer not in the Graphing Quadratics design meeting, phetsims/graphing-quadratics#112. @arouinfar could you please review the proposed change for the look of a disabled slider (see #478 (comment)), and let us know if you have any concerns?

@ariel-phet
Copy link

@pixelzoom I think myself, @arouinfar, @amanda-phet, and @kathy-phet would be the current stakeholders. What have you have done looks correct to me, and what @kathy-phet and company seemed to have in mind.

@ariel-phet ariel-phet removed their assignment Mar 5, 2019
@arouinfar
Copy link
Contributor

Thanks for checking @pixelzoom. I think the proposal with reduced opacity sliders looks nice, and would agree that it is more consistent with other disabled UI components.

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar Mar 5, 2019
@pixelzoom
Copy link
Contributor Author

Thanks everyone for the feedback. I will proceed with this change.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 5, 2019

This requires removing enabledProperty from the SliderTrack and SliderThumb APIs. SliderTrack is used exclusively by Slider, but SliderThumb is also used as a supertype by COULOMBS_LAW/ChargeControlSliderThumb. So I've have to investigate the impact on Coulomb's Law.

EDIT: Discussed with @jessegreenberg on Slack. Looks like enabledProperty is a dummy Property in COULOMBS_LAW/ChargeControlSliderThumb. The slider (and thumb) are never disabled in this sim.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 5, 2019

Sims that were modified and tested:

  • capacitor-lab-basics
  • coulombs-law
  • gravity-and-orbits
  • gravity-force-lab
  • hookes-law
  • masses-and-springs
  • molarity
  • molecules-and-light
  • ohms-law
  • pendulum-lab
  • resistance-in-a-wire
  • rutherford-scattering
  • scenery-phet demo
  • states-of-matter (atomic-interactions)
  • tambo

pixelzoom added a commit to phetsims/masses-and-springs-basics that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/rutherford-scattering that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/capacitor-lab-basics that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/resistance-in-a-wire that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/molecules-and-light that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/masses-and-springs that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/gravity-and-orbits that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/gravity-force-lab that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/states-of-matter that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/pendulum-lab that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/coulombs-law that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/hookes-law that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/molarity that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/ohms-law that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/tambo that referenced this issue Mar 5, 2019
pixelzoom added a commit that referenced this issue Mar 5, 2019
pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Mar 5, 2019
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 5, 2019

Changes completed, sims listed in #478 (comment) tested by comparing to published versions.

The changes made are as summarized in #478 (comment).

@ariel-phet please assign a developer to review.

@samreid
Copy link
Member

samreid commented Jun 11, 2020

I skimmed the change sets and tested in the sun test harness. All looks well. I haven't been following the EnabledComponent mixin too closely, should Slider be using that? If so, would you like to work on that here or in a separate issue?

@samreid samreid assigned pixelzoom and unassigned samreid Jun 11, 2020
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 12, 2020

Converting other components (like Slider) to use EnabledComponent is the subject of #585. So if all looks well, I'm going to close this issue.

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

4 participants