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

Leading and trailing separators still visible #123

Closed
samreid opened this issue Mar 21, 2023 · 3 comments
Closed

Leading and trailing separators still visible #123

samreid opened this issue Mar 21, 2023 · 3 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Mar 21, 2023

According to the scenery documentation https://phetsims.github.io/scenery/doc/layout#separators leading and trailing separators should be invisible. However, in My Solar System if I hide the checkbox before the first separator like this:

Subject: [PATCH] Standardize author annotations
---
Index: js/common/view/MySolarSystemControls.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/MySolarSystemControls.ts b/js/common/view/MySolarSystemControls.ts
--- a/js/common/view/MySolarSystemControls.ts	(revision 03cf57190db382b349faa663de190cb23b3bb8d6)
+++ b/js/common/view/MySolarSystemControls.ts	(date 1679411791931)
@@ -33,7 +33,7 @@
 
     super( {
       children: [
-        ...createOrbitalInformationCheckboxes( model, providedOptions.tandem ),
+        // ...createOrbitalInformationCheckboxes( model, providedOptions.tandem ),
         new HSeparator( SolarSystemCommonConstants.HSEPARATOR_OPTIONS ),
         ...createArrowsVisibilityCheckboxes( model, providedOptions.tandem ),
         new HBox( {

The separator is still visible (above the speed checkbox):

image

@jonathanolson
Copy link
Contributor

The layoutOptions in SolarSystemCommonConstants.HSEPARATOR_OPTIONS is overriding the normal separator options (where it specifies that it is a separator).

Thinking about the best way to handle this.

@jonathanolson
Copy link
Contributor

Fix pushed above. However it's a bit inconvenient that the layout options aren't combinable. I wonder if it should switch to a model where it mutates by default (instead of... setting it as an entire object). Thoughts?

@samreid
Copy link
Member Author

samreid commented Mar 21, 2023

In the commit, @AgustinVallejo exported the default separator layout options so it is possible for clients to extends rather than replace them. Closing.

@samreid samreid closed this as completed Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants