Skip to content
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 border issue #2074

Merged
merged 2 commits into from
Mar 16, 2023
Merged

fix border issue #2074

merged 2 commits into from
Mar 16, 2023

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Mar 15, 2023

Fixes an issue where a border was partially black. I'd seen this before but it was a mystery why until recently...

You can see the issue in this snapshot. Note the inner, tall, and wide borders.

Screenshot 2023-03-15 at 21 36 10

Also changed the test for border opacity, which also exhibited the error. Note how the border was black with 0% opacity, where it should have been invisible.

Screenshot 2023-03-15 at 21 45 00

@@ -1,43 +1,40 @@
from textual.app import App, ComposeResult
from textual.containers import Grid
from textual.widget import Widget
from textual.containers import Vertical
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote this snapshot test, because it wasn't clear to me what it should look like.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see with the rewrite is that it doesn't exercise the CSS parser in quite the same way. The original test was designed to try out a good combination of styles, colours and alpha values to ensure it all worked together.

That looks to have been lost.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. But those are better captured with a unit test. Snapshots should be used to validate that things look correct only, where you can't tell from the data alone.

Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barring Dave's comment on the snapshot test, looks good to me.

@willmcgugan willmcgugan merged commit 43253f5 into main Mar 16, 2023
@willmcgugan willmcgugan deleted the border-fix branch March 16, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants