-
Notifications
You must be signed in to change notification settings - Fork 3
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
add list of list support to buttongroup widget #90
Merged
mhthies
merged 7 commits into
mhthies:main
from
jo47011:add_nested_groups_to_buttongroup
Oct 30, 2024
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d56cfce
add list of list support to buttongroup widget
jo47011 cf94d0e
remove package-lock.json
jo47011 6a93233
fix: flake issues
jo47011 97fd8b1
fix: tests
jo47011 920f207
add: parameter description details
jo47011 531087b
add: button group test
jo47011 f017ccd
fix: flake issue
jo47011 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,13 @@ | ||
{% from "buttons.inc.htm" import render_button %} | ||
<label class="shc label float-btn">{{ label }}</label> | ||
<div class="ui buttons right floated"> | ||
{% for button in buttons %} | ||
{{ render_button(button) }} | ||
<div class="ui spaced horizontal list right floated"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the use of |
||
{% for button_group in button_groups %} | ||
<div class="item"> | ||
<div class="ui buttons"> | ||
{% for button in button_group %} | ||
{{ render_button(button) }} | ||
{% endfor %} | ||
</div> | ||
</div> | ||
{% endfor %} | ||
</div> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea why
mypy
wants this now here and not at other places, e.g.ui_showcase.py
. Maybe you have an idea how to solve it without the cast. Looks kind of ugly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MyPy tries to guess the type of that list here, based on the first common ancestor class of the two buttons it finds. Unfortunately it finds the common ancestor
WebDisplayDatapoint[bool]
instead ofAbstractButton
. So it complains that we are passing a list of ``WebDisplayDatapoint[bool]s instead of a list of
AbstractButtons`. I assume that, without the new `Union` in the parameter type hint, MyPy was smart enough to find that `List[AbstractButton]`, as expected by the parameter, is also a valid type of that list, so it used that instead of trying to guess the type.I think, the "correct" solution would be to explicitly type-hint the list:
However, that also looks kind of ugly, especially in the example code. So maybe, we go for a simple type-ignore with a comment, like
# type: ignore # MyPy has problems with guessing the correct type of the list of buttons
.Maybe, MyPy will learn this type inference eventually and we can remove the comment. (I will check that once in a while by running MyPy with
--warn-unused-ignores
locally.)