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

Pain points for "divider". #1448

Closed
pixelzoom opened this issue Sep 9, 2022 · 7 comments
Closed

Pain points for "divider". #1448

pixelzoom opened this issue Sep 9, 2022 · 7 comments

Comments

@pixelzoom
Copy link
Contributor

A pain point I've encountered when adding support for dynamic locale is having to deal with separators in control panels. For example, see phetsims/molecule-polarity#147 and phetsims/natural-selection#325.

To address this, the developer needs to make this change:

HSeparator -> VDivider (documented as "a horizontal line")
VSeparator -> HDivider (documented as "a vertical line")

Pain points:

(1) The names for the new "dividers" are super confusing, because they don't match the orientation of their line, or their documentation. The names are HDivider ("a vertical line") and VDivider ("a horizontal line"). And developers need to change HSeparator to VDivider, and VSeparator to HDivider. Very confusing, and this sent me down a rabbithole.

(2) Why the name change from "separator" to "divider"? This will change the PhET-iO API wherever a separator is instrumented - as is the case in molecule-polarity, natural-selection, ...

(3) Why do we need new "divider" classes? Why not reimplement HSeparator and VSeparator so that developer don't need to do anything to address this? (And feel free to move HSeparator and VSeparator is scenery.)

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Sep 10, 2022

I've had to change the PhET-iO API for 3 sims so far, about to have to change a 4th. And I suspect that I'll be undo those changes when @kathy-phet and @arouinfar weigh in. So I'm going to pause work on dynamic layout that is related to separator, and raise priority.

@kathy-phet @arouinfar see (2) above:

(2) Why the name change from "separator" to "divider"? This will change the PhET-iO API wherever a separator is instrumented - as is the case in molecule-polarity, natural-selection, ...

@arouinfar
Copy link

(2) Why the name change from "separator" to "divider"? This will change the PhET-iO API wherever a separator is instrumented - as is the case in molecule-polarity, natural-selection, ...

This was not a change that was requested by designers. I prefer the term "separator", and it appears frequently in PhET-iO sims. Is there any reason why we can't keep it that way?

Also, I want to note that GAO includes "separator" terminology, so this issue is fairly time-sensitive.

@arouinfar arouinfar removed their assignment Sep 12, 2022
@pixelzoom
Copy link
Contributor Author

Also, I want to note that GAO includes "separator" terminology, so this issue is fairly time-sensitive.

It looks like GAO is using the old separators, not the new dividers. And it's able to do that by having fixed-size control panels that do not resize based on locale.

@jonathanolson
Copy link
Contributor

(1) The names for the new "dividers" are super confusing

Agreed, apologies about that. I'll get that changed over.

(2) Why the name change from "separator" to "divider"?

Because of (3) having a separate type for each. I didn't want to have "sun/HSeparator" AND "scenery/HSeparator"

(3) Why do we need new "divider" classes? Why not reimplement HSeparator and VSeparator so that developer don't need to do anything to address this? (And feel free to move HSeparator and VSeparator is scenery.)

I'll look into whether this is possible. "Separators" REQUIRE a size, and "dividers" SHOULD NOT have a size, so I could perhaps "mash together" the implementations with an overloaded constructor (e.g. if you provide a number as a first parameter, do all of the "separator" things, otherwise do all of the "divider" things). That seems a bit unclean. Thoughts on that? (One has a static size, the other includes the sizable mixin, has certain layoutOptions, and links the preferred size - all of these things CAN be handled conditionally)

So I guess it comes down to "we can use the same type for new/old, however the behavior of the separator COMPLETELY changes based on whether you provide a number or not in the constructor signature". @pixelzoom thoughts?

@pixelzoom
Copy link
Contributor Author

Some options:

(1) Rename HSeparator and VSeparator to HSeparatorDeprecated and VSeparatorDeprecated. Rename VDivider and HDivider to HSeparator and VSeparator. Probably the easiset option.

(2) Do what you suggested above. If there's a first numeric arg, it's the static length of the separator. The downside is a developer will need to remember to remove this arg for cases where the separator should resize.

(3) Like (2), but the first arg is the initial length, not a static length. For separators that are children of a layout Node, make them resize as they should. Downside is that we'll still be computing an unnecessary first arg in a some (many?) cases.

My preference is (1).

marlitas added a commit to phetsims/fourier-making-waves that referenced this issue Sep 13, 2022
marlitas added a commit to phetsims/ratio-and-proportion that referenced this issue Sep 13, 2022
marlitas added a commit to phetsims/acid-base-solutions that referenced this issue Sep 13, 2022
marlitas added a commit to phetsims/gravity-and-orbits that referenced this issue Sep 13, 2022
marlitas added a commit to phetsims/circuit-construction-kit-common that referenced this issue Sep 13, 2022
marlitas added a commit to phetsims/blackbody-spectrum that referenced this issue Sep 13, 2022
marlitas added a commit to phetsims/graphing-quadratics that referenced this issue Sep 13, 2022
marlitas added a commit to phetsims/density-buoyancy-common that referenced this issue Sep 13, 2022
@marlitas
Copy link
Contributor

@jonathanolson and I changed the names of files and usages as following:

  • HSeparator => HSeparatorDeprecated
  • VSeparator => VSeparatorDeprecated
  • VDivider => HSeparator
  • HDivider => VSeparator

Assigning back to @pixelzoom for feedback/review. Feel free to close if all looks good!

@pixelzoom
Copy link
Contributor Author

Looks great, thanks! 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

5 participants