search

phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

PhET-iO support for choosing to hide tabs on the preferences dialog #860

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

From https://github.com/phetsims/joist/issues/744#issuecomment-1247304108

@jessegreenberg could you please help me with a facility to hide the tabs. We want to be able to customize if certain tabs are available to the user. If you hide the currently selected tab, it would also hide the panel being shown (because it would likely move to the next selection.

I think some sort of model Property that guides both the tabs and panels makes the most sense. Perhaps the PreferencesModel could have a single Property, like "availableTabsProperty", or each sub-model could have a "tabDisplayedProperty". What do you think? Do you mind taking the lead on this because I am unsure based on the complexities of tabs working with panels etc.

samreid commented 2 years ago

At today's meeting, @kathy-phet @arouinfar and I discussed that using visibleProperty = false to hide the tabs seems like a good starting place.

The tabs should be nested together in a node in the studio tree, like "tabs"? The children would be like tabs.overviewTab, tabs.visualTab, tabs.localizationTab.

Will all of the unused tabs be instrumented but invisible? We are unsure about that.

jessegreenberg commented 2 years ago

@samreid and I discussed over slack:

SR: Are the tabs in a HBox? Then it will automatically reflow and visibleProperty will work great.

JG: Yep! Sounds good. From the issue Will all of the unused tabs be instrumented but invisible? We are unsure about that. Tabs are only included if the sim has features that are supported in that tab. I was thinking only used tabs would be available but they could be made invisible with PhET-iO. Is that OK?

SR: Sounds great to me, thanks!

jessegreenberg commented 2 years ago

It looks like the instrumentation of PreferencesTabs and PreferencesPanels is possible so that they can be hidden with visibleProperty. But then they would have to be set together (separately). A single Property like @zepumph suggested would probably be a better API.

PreferencesTabs has a supportedTabs I think it would be good to make that an instrumented supportedTabsProperty. I see something very similar in Sim.ts at availableScreensProperty, Ill take a first stab doing a mimic of that.

jessegreenberg commented 2 years ago

Discussed with @samreid, the API expectation was to go through visibleProperty so we should continue to do that. Right now visibility of each panel is tied to selectedTabProperty but we should try also linking it to visibility of the PreferencesTab so that the API just goes through that Node.

jessegreenberg commented 2 years ago

https://github.com/phetsims/joist/issues/867 should have the visibility Properties linked up correctly so all that should be left is to add tandems to the Tabs so their visibility can be set by PhET-iO.

jessegreenberg commented 2 years ago

Alright, I think we are mostly implemented as requested. I had a couple of design questions.

I am not sure if these are use cases we care about.

@samreid can you please review the instrumentation in and verify the behavior in studio is OK (or possibly reassign that part to design team).

samreid commented 2 years ago

What if a tab is set to visible: false while that tab is currently displayed? Do we need to display a different one?

Yes, in the top comment, @zepumph said: "If you hide the currently selected tab, it would also hide the panel being shown (because it would likely move to the next selection."

  • What if all the tabs are set to invisible?

Maybe just have an empty dialog box?

samreid commented 2 years ago

I added a commit that switches tabs to the leftmost visible tab if the currently selected tab becomes hidden. If all tabs are hidden, it is just an empty dialog box. @jessegreenberg can you please review?

arouinfar commented 2 years ago

@samreid @jessegreenberg I stumbled across these changes when reviewing another issue, and I just wanted to let you both know that the behavior in master looks really good. The preferencesTabs.*Tab.visibleProperty structure is clear and it behaves as I would expect. The edge cases @samreid addressed in https://github.com/phetsims/joist/issues/860#issuecomment-1255744662 also look good.

If further commits are made and need design re-review, feel free to assign to me. Otherwise, consider this the design sign-off. :)

jessegreenberg commented 2 years ago

Thanks @samreid, that multilink is great and works as described. In the above commit I added the unmultilink to the dispose which I think is necessary because those Properties are not owned by the PreferencesTabs.

With @arouinfar design sign off I think we are done for this issue. Closing.