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 Toggle Buttons on a selection list #2930

Merged
merged 6 commits into from
Jul 17, 2023
Merged

Conversation

dmunozv04
Copy link
Contributor

@dmunozv04 dmunozv04 commented Jul 12, 2023

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@dmunozv04
Copy link
Contributor Author

This PR makes the Toggle Buttons on a selection list clickable/hoverable by adding the style meta option property

@davep
Copy link
Contributor

davep commented Jul 12, 2023

Good catch. Would you mind adding a CHANGELOG entry to that effect too?

I sense we should also add an extra unit test or two to ensure that clicking on the checkbox has the desired effect (I'm happy to help with that if you're not familiar with adding tests to Textual).

@davep davep added the bug Something isn't working label Jul 12, 2023
@dmunozv04
Copy link
Contributor Author

dmunozv04 commented Jul 12, 2023

As I’m not familiar with adding tests, I’d appreciate getting help on doing that
EDIT: I’ve updated the changelog

@davep
Copy link
Contributor

davep commented Jul 12, 2023

I’ve updated the changelog

Thanks.

As I’m not familiar with adding tests, I’d appreciate getting help on doing that

No worries; when I get a moment I'll add an appropriate test for this.

davep added 2 commits July 12, 2023 16:09
Mainly updating the now-external snapshot testing code so that the failure
report (which was to be expected) gets generated.
@davep
Copy link
Contributor

davep commented Jul 12, 2023

Added an update of the dependencies to this PR as Textualize/pytest-textual-snapshot#1 was needed so that snapshot tests could be run and an error report generated; the change here (correctly) caused snapshot tests to fail as there's new metadata kicking around.

Also then regenerated the snapshot tests.

@davep
Copy link
Contributor

davep commented Jul 12, 2023

Right, that all looks like it's going through okay; I'll see about adding the extra test in the morning.

Thanks again for this @dmunozv04 -- good spot!

@davep
Copy link
Contributor

davep commented Jul 13, 2023

Tests added; as I've added code to this too I've requested review from the rest of the team.

@willmcgugan willmcgugan merged commit 4d699c8 into Textualize:main Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants