CheckboxList & more Dialogs doc #889
Conversation
|
Off-topic question: how did you generate the graphics used here, for instance: Is it a plain screenshot or something fancier ? I can't get anything as fancy :/ I'd be happy to add such graphics to the part of the doc about |
Codecov Report
@@ Coverage Diff @@
## master #889 +/- ##
==========================================
+ Coverage 71.06% 71.08% +0.01%
==========================================
Files 141 141
Lines 13378 13406 +28
==========================================
+ Hits 9507 9529 +22
- Misses 3871 3877 +6
Continue to review full report at Codecov.
|
gpotter2
changed the title
|
Now that #880 has been merged (great job by the way !), this should be ready for reviewing. I've rebased & re-tested everything. |
|
Hi @gpotter2, Very nice. I like what you have. There are a few remarks however I added that need to be fixed before merging. It also needs to pass mypy, except for the "_ already defined warnings". About the screenshots, I think I used the normal Terminal application on OS X, with some changes in the settings to not display anything in the title bar. Then I used the command-shift-4 screenshot to capture the Window. This will automatically create the shadow behind the window. The screenshots were made on a retina screen with a font size sufficiently big for the quality. |
I'm sorry, I'm not sure what you mean :/ I don't see anything Also, the tests in the last commit seem to pass, if I'm not mistaken. I made sure I followed the mypy guidance you now follow. |
prompt_toolkit/widgets/base.py
Outdated
| result: StyleAndTextTuples = [('', '[')] | ||
| result.append(('[SetCursorPosition]', '')) | ||
| class _DialogList(Generic[_T]): | ||
| class_style_container = None |
There was a problem hiding this comment.
These would need type annotations. Something like:
class_style_container: str = ''or
class_style_container: Optional[str] = NonePersonally, I prefer to use empty strings rather then None in this case, because an empty string is a valid style string.
An empty string is a good default for something that isn't styled.
Further, I'd like these to be renamed into:
container_style
default_style
selected_style
checked_style
The reason for that is that a style string doesn't have to contain a class name. It can contain inline styling as well, like fg:red.
| multiple_selection = True | ||
|
|
||
|
|
||
| class Checkbox(CheckboxList): |
There was a problem hiding this comment.
I like this way of support a single Checkbox. Well done.
prompt_toolkit/widgets/base.py
Outdated
| def __init__(self, values: List[Tuple[_T, AnyFormattedText]]) -> None: | ||
| assert self.class_style_container is not None |
There was a problem hiding this comment.
The four is None assertions can be removed if we use empty strings as defaults.
prompt_toolkit/widgets/base.py
Outdated
| assert len(values) > 0 | ||
|
|
||
| self.values = values | ||
| self.current_value: _T = values[0][0] | ||
| if self.multiple_selection: | ||
| self.current_values = [] |
There was a problem hiding this comment.
I think current_values is only assigned here if multiple_selection is set. Mypy's not going to like that. We should always assign it, even when it's not used, and add a type annotation.
self.current_values: List[_T] = []
prompt_toolkit/widgets/base.py
Outdated
| if self.multiple_selection: | ||
| self.current_values = [] | ||
| else: | ||
| self.current_value: _T = values[0][0] |
There was a problem hiding this comment.
The same for current_value. It should always be assigned. (Maybe add a comment saying it's only used in case of multiple_selection.)
prompt_toolkit/widgets/base.py
Outdated
| selected = (i == self._selected_index) | ||
|
|
||
| style = '' | ||
| if checked: | ||
| style += ' class:radio-checked' | ||
| style += ' %s' % self.class_style_checked |
There was a problem hiding this comment.
No string interpolation should be required here. The style is supposed to be a string already (mypy can check whether that's always the case).
prompt_toolkit/widgets/base.py
Outdated
|
|
||
| result.append((style, '(')) | ||
| result.append((style, self.class_style_open)) |
There was a problem hiding this comment.
I think class_style_open should be called something else. Like open_character (it's not a classname).
Further, this variable should be added to the base class.
|
@gpotter2, I forgot indeed to submit the review. Thanks for reminding me! |
…not beeing applied
|
Thanks for the follow up, I've updated the PR accordingly (+rebased against master) |
|
Thank you @gpotter2, I really appreciate it! |
|
Is there any documentation for this functionality? |
|
There is but it looks like the automated readthedocs builds have been turned off: current version "master" is pretty old You'll have to browse it on GitHub: https://github.com/prompt-toolkit/python-prompt-toolkit/blob/master/docs/pages/dialogs.rst |
Hey,
I've implemented what the TODO suggested. I've tested the code so it should work fine with existing implementations.
This PR:
CheckboxList, which has a shared implementation withRadioListCheckboxuseCheckboxListradiolist_dialogandcheckboxlist_dialogdocs/pages/dialogs.rstto document the available styling classes + examples.run()calls in the dialog examples from the doc, as of v3.0frame-labelinstead offrame.labelas in the code:python-prompt-toolkit/prompt_toolkit/widgets/base.py
Line 415 in 237cf46
The image linked to this example is the wrong, as one style wasn't applied :/
Demo
This PR: