-
Notifications
You must be signed in to change notification settings - Fork 764
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
fix(footer): enable spacing between key and description #4651
fix(footer): enable spacing between key and description #4651
Conversation
I'm actually now wondering if this whole section should be adapted for CSS styling with padding and margins: textual/src/textual/widgets/_footer.py Lines 91 to 98 in e9ad400
Maybe something like this. Rather than hardcoding What do you think? label_text = Text.assemble(
# Key display (with padding)
(" " * key_padding.left, key_style),
(key_display, key_style),
(" " * key_padding.right, key_style),
# Add spacing to right of the key
(" " * key_margin.right),
# Description display
(description, description_style),
# Add spacing to right of the description
(" " * description_margin.right),
) |
Seems reasonable. Although one thing to be aware of is that margins overlap. So the space between key and description should be the maximum of the two margins. |
I had forgotten you can't 'fine-tune' the padding/margin styles which might complicate things. For example, if the current margin was defined in the Footer .footer-key--description {
margin-right: 1;
} And you tried to also add space between key and description in your CSS like this: .footer-key--description {
margin-left: 1;
} This apparently causes all the individual margin values to reset, so the space to the right of the description is lost. You would also need to explicitly set the |
Sorry also just to note after diving deeper into this: I spotted the 'default' footer uses a raw space after the description, whereas in the 'compact' style instead uses the grid gutter. textual/src/textual/widgets/_footer.py Lines 123 to 125 in 4a40027
This could be simplified to always use the grid gutter, but I'm conscious this would cause snapshots tests to fail, especially after the recent spate of other changes in Textual. |
This reverts commit 36ecb04.
I've reverted the commit that updated the snapshot tests. I wanted to check if #4675 might account now for these changes but unfortunately not. |
…enable-padding-between-key-and-description
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.
Seems like there's a rendering issue - see my other comment. Would be good to investigate this before merging.
Good catch, thanks Darren! I might be wrong but is the fix as simple as adding Let me know and I'm happy to add a snapshot test for this and update! |
@TomJGooding Sadly that doesn't seem to fix it 😢 |
Sorry to clarify, I mean the |
Thanks Darren, good catch again as I hadn't spotted I'd changed the hover styling. I expect that is why the "the 'default' footer uses a raw space after the description, whereas in the 'compact' style instead uses the grid gutter" then! |
Yeah I'm guessing that's why. I tried playing around with the CSS for a few minutes and seem to get every combination of spacing apart from the one I'm aiming for 😂 |
Thanks again Darren for catching these issues, in hindsight I probably should have added more snapshot tests before making changes. I will double-check with a fresh head tomorrow, but hopefully these are resolved with my latest commits. |
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.
Looks good, thanks!
This is my (second) attempt to fix #4557. It isn't pretty but I'm still not sure how this should be implemented. Perhaps I'm missing something obvious, but the fact that the Footer currently uses 'raw' spaces suggests adding padding might not be so simple?
Please review the following checklist.