oliversalzburg / cbc-kitten-scientists

Add-on for the wonderful incremental browser game: http://kittensgame.com/web/
MIT License
8 stars 3 forks source link

feat: expand/collapse all options #116

Closed Jian-Wuyou closed 2 years ago

Jian-Wuyou commented 2 years ago

Description

Put every other major option inside a list (#items-list-engine) under the "Kitten Scientists" (#ks-engine) list element. Only issue is that the sidebar width shrinks when the options are collapsed (not sure what causes it).

The KS script still loads and saves settings properly, and automation tasks (crafting/building/etc.) are still working as intended.

Related issues

Resolves #109

Images

image

image

oliversalzburg commented 2 years ago

Thanks. That's a nice change in general. However, I believe #109 wants that all sub-sections are expanded/collapsed to quickly make changes to all sections. I don't see this solved here.

I really like using the _getItemsToggle() element to control this behavior. Maybe capture the click event and manually expand/collapse the sub-sections? I would be fine with the controls on the main "Enable Scientists" toggle to behave slightly different than the sections below.

Jian-Wuyou commented 2 years ago

Well, I was also thinking of a few ways to address the issue: put every other option under Enable Kitten Scientists (the PR above) or add code that will toggle everything else (basically add a "display: none" to each list item without actually changing the list structure).

I believe https://github.com/oliversalzburg/cbc-kitten-scientists/issues/109 wants that all sub-sections are expanded/collapsed to quickly make changes to all sections. I don't see this solved here.

Then i guess the second option is a better fit in this case. Will make a PR for that later and just compare which one is preferred :)

oliversalzburg commented 2 years ago

Maybe you can also just trigger the expand/collapse on the section directly and encapsulate the logic centrally.

I could imagine having a expand() and collapse() on the SettingsSectionUi itself. And then we could call that from the items toggle and from the main section to batch-expand/collapse all sections.

Jian-Wuyou commented 2 years ago

Hmm, in SettingsSectionUi the function _getSettingsPanel() uses the same _itemsExpanded value across all calls for each instance of SettingsSectionUi (i.e. it assumes that there is only one mainChild per object that needs toggling). So I was thinking of just adding hide(), show(), and toggle() as public methods of SettingsSectionUi and storing mainChild as a protected variable.

Also possible to just remove _itemsExpanded entirely and just use .is(':visible') and .is(':hidden') to check the state when toggling

oliversalzburg commented 2 years ago

I like the first part about the API, but I prefer to keep state primarily in member variables and then only reflect those into the DOM.

I don't want the state management code to be too closely related to the DOM or jQuery, in case the entire UI would be moved to a more state-focused approach, like React or lit.

Jian-Wuyou commented 2 years ago

Made a different PR since the code changes there encompass the feature implemented in this PR.