Added collapse/expand blocks for "advanced" settings #336
Conversation
|
@joaquin30 looking good so far! Grabacion.de.pantalla.desde.2024-12-16.09-28-57.webmI know this is still draft, but for the 1st review make sure that this pass the linting and formatting check above. You can do that locally if you setup pre-commit as in the README |
There was a problem hiding this comment.
This is working nicely! I have left suggestions for improving the code.
Besides the improvement requests, we try to have descriptive commit messages in this project. Could you squash your commits and provide a good description? For example:
- A first line explaining briefly what's this about.
- A description of what does this PR do and how it's implemented.
- As last line, the URL to the issue that this fixes, eg "Fixes https://..."
| # separate the template string by | character | ||
| # for normal and advanced parameters (advanced parameters are hidden) |
There was a problem hiding this comment.
Please capitalize the comment sentence and add . to the end of the sentence.
| # separate the template string by | character | |
| # for normal and advanced parameters (advanced parameters are hidden) | |
| # Separate the template string by | character | |
| # for normal and advanced parameters (advanced parameters are hidden). |
| # Parse the advanced part of the template string | ||
| for regex_match in _display_template_regex.search_all(template_string_advanced): | ||
| if regex_match.names.has("label"): | ||
| var label_string := regex_match.get_string("label") | ||
| items.append({"label": label_string, "hidden": true}) | ||
| elif regex_match.names.has("in_parameter"): | ||
| var parameter_string := regex_match.get_string("in_parameter") | ||
| items.append({"in_parameter": _parse_parameter_format(parameter_string), "hidden": true}) | ||
| elif regex_match.names.has("out_parameter"): | ||
| var parameter_string := regex_match.get_string("out_parameter") | ||
| items.append({"out_parameter": _parse_parameter_format(parameter_string), "hidden": true}) | ||
| elif regex_match.names.has("const_parameter"): | ||
| var parameter_string := regex_match.get_string("const_parameter") | ||
| items.append({"const_parameter": _parse_parameter_format(parameter_string), "hidden": true}) |
There was a problem hiding this comment.
Except for the "hidden": true, this for loop is exactly the same as the one above. So please come with a inner function and a boolean parameter.
| $CollapseButton.connect("button_up", collapse) | ||
| $ExpandButton.connect("button_up", expand) |
There was a problem hiding this comment.
For these buttons, please declare the nodes at the top of the script. And also add unique names to them. This way the script doesn't depend on UI changes:
@onready var _collapse_button: Button = %CollapseButton
@onready var _expand_button: Button = %ExpandButton
| for item in BlockDefinition.parse_display_template(format_string): | ||
| var hidden = item.has("hidden") and item.get("hidden") |
There was a problem hiding this comment.
item.get("hidden") already returns null if the key is missing, and you can also pass a default. So you can probably do the same without the has() check:
| var hidden = item.has("hidden") and item.get("hidden") | |
| var hidden = item.get("hidden", false) |
Untested, please confirm!
| _append_label(collapsable if hidden else _container, item.get("label")) | ||
| elif item.has("in_parameter"): | ||
| _append_input_parameter(item.get("in_parameter"), match_id) | ||
| _append_input_parameter(collapsable if hidden else _container, item.get("in_parameter"), match_id) | ||
| elif item.has("out_parameter"): | ||
| _append_output_parameter(item.get("out_parameter"), match_id) | ||
| _append_output_parameter(collapsable if hidden else _container, item.get("out_parameter"), match_id) | ||
| elif item.has("const_parameter"): | ||
| _append_const_parameter(item.get("const_parameter"), match_id) | ||
| _append_const_parameter(collapsable if hidden else _container, item.get("const_parameter"), match_id) |
There was a problem hiding this comment.
The collapsable if hidden else _container is repeated 4 times. Is better to move it to a variable before the if/elif/elif/elif block.
This commit modifies the display_template parsing of .tres block definitions, adding a separator using the | character. All labels and parameters after this separator are added to a new CollapsableSettings container that has collapse/expand buttons, with the goal of hiding advanced settings from users. To do this, a new hidden key with a boolean value is added to the results of the parse_display_template method. Additionally, CollapsableSettings is implemented using an HBoxContainer and changing the visibility of child nodes when changing collapse/expand states. Fixes endlessm#288.
joaquin30
force-pushed
the
collapse-expand-advanced-settings
branch
from
075f9ee to
7dcd95a
Compare
/claim #288
Closes #288. This adds a button for expand/collapse settings in blocks that are after the "|" separator.