Add DigitDisplay widget #2995
Add DigitDisplay widget #2995
Conversation
eliasdorneles
force-pushed
the
add-digit-display
branch
from
0eff2d5 to
d62daec
Compare
| @@ -10,7 +10,7 @@ unit-test: | |||
|
|
|||
| .PHONY: test-snapshot-update | |||
| test-snapshot-update: | |||
| $(run) pytest --cov-report term-missing --cov=textual tests/ -vv --snapshot-update | |||
| $(run) pytest --cov-report term-missing --cov=textual tests/ -vv --snapshot-update $(PYTEST_ARGS) | |||
There was a problem hiding this comment.
Note to reviewer: I took the liberty to add this snippet, which let you do things like: make PYTEST_ARGS="-k digit_display" test-snapshot-update while developing.
Let me know if you prefer that I remove this.
|
Screenshot (on Ubuntu - Terminator):
There are two example apps:
@rodrigogiraoserrao @willmcgugan what do you think? |
eliasdorneles
force-pushed
the
add-digit-display
branch
from
2495d72 to
fc5d189
Compare
|
@rodrigogiraoserrao thanks for the review! I've updated it taking into account your comments, also improved the screenshot of the reactive example by adding a few keys pressed =) |
There was a problem hiding this comment.
It's looking good to me.
You left two of my comments without a response; I'm not sure if it is because you disagree (which is fine, please argue against me) or if you just missed them.
@willmcgugan will take a look and then we'll get back to you :)
Oops, i think i missed them, lemme check again! |
eliasdorneles
force-pushed
the
add-digit-display
branch
from
a67ed63 to
8a3d411
Compare
|
Updated, I think I got everything now, lemme know if there is still something missing. |
eliasdorneles
force-pushed
the
add-digit-display
branch
from
8a3d411 to
5721933
Compare
| } | ||
| """ | ||
|
|
||
| def __init__(self, initial_value=" ", **kwargs): |
| } | ||
| """ | ||
|
|
||
| def __init__( |
| } | ||
|
|
||
|
|
||
| class _SingleDigitDisplay(Static): |
There was a problem hiding this comment.
Docstring please (we like them for private classes also).
| } | ||
|
|
||
|
|
||
| class _SingleDigitDisplay(Static): |
There was a problem hiding this comment.
Cat you add get_content_width and get_content_height methods. This allows "auto" dimensions pick a more optimal code path. There should be plenty of examples in other widgets...
There was a problem hiding this comment.
Thanks for the review!
I've updated following up to your other comments, and added an implementation of the methods you mention here.
I'm not sure how best to test the "auto dimensions" thing you mention, can you have a look?
There was a problem hiding this comment.
Hello @willmcgugan 👋
Were you able to have a look at this?
There was a problem hiding this comment.
If you set your widget to width: auto; and height: auto it should call get_content_width etc.
eliasdorneles
force-pushed
the
add-digit-display
branch
from
5721933 to
b6e9580
Compare
eliasdorneles
force-pushed
the
add-digit-display
branch
from
b6e9580 to
982d244
Compare
|
Just rebased, to be synced w/ latest code. |
| self.update(content) | ||
|
|
||
| def get_content_width(self, container: Size, viewport: Size) -> int: | ||
| return self._content_width |
There was a problem hiding this comment.
Are all the digits not the same size? If so, could we just return 3 here?
There was a problem hiding this comment.
I was trying to make the dot and comma to change width to 2, but in the end it's simpler to have every digit the same size indeed 👍
Lemme update this!
|
Thanks for your work on this! We've decided to go with a slightly different implementation here (#3073). It's not a reflection on your work (which is excellent) -- we've taken a different approach to the design. We used this PR as inspiration... Thanks for joining in the sprint! Will |
|
Hi @willmcgugan, No worries, I understand. I admit I do feel a bit disappointed that my work hasn't made the cut. At the same time, it's very interesting for me to see the differences between our designs and to learn from yours. 🤓 Anyway, I've got some experience working on open source projects, enough to understand your position, I know that it's important for a maintainer to know how to say no. You did that gently and kindly, thank you for that! 💜 Have a good day and keep up the great work! ☀️ |
This supersedes: #2990
Please review the following checklist.