Skip to content

feat: add rule widget#3209

Merged
willmcgugan merged 18 commits intoTextualize:mainfrom
TomJGooding:feat-add-rule-widget
Sep 4, 2023
Merged

feat: add rule widget#3209
willmcgugan merged 18 commits intoTextualize:mainfrom
TomJGooding:feat-add-rule-widget

Conversation

@TomJGooding
Copy link
Copy Markdown
Collaborator

@TomJGooding TomJGooding commented Aug 30, 2023 •
edited
Loading

Description

Draft PR to add a Rule widget - closes #2982. This takes inspiration from the good work already done in #3060.

As discussed in the linked issue, this adds a single Rule widget rather than separate widgets for horizontal and vertical orientations.

@rodrigogiraoserrao This is still a work in progress, but I just wanted to double-check I was on the right track with this? Especially with the different line_styles for the rule where I'm directly using the existing border styles?

Related Issue

How Has This Been Tested?

TODO! Snapshot tests added for the horizontal and vertical rule examples. Also added tests that an invalid rule orientation or line style will raise an exception.

Screenshots (if appropriate):

Horizontal rules:

image

Vertical rules:

image

Checklist:

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

rodrigogiraoserrao
rodrigogiraoserrao reviewed Aug 31, 2023
View reviewed changes
Copy link
Copy Markdown
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.

This is looking great to me. I didn't think of using the border edge styles as styles for the rule but that makes a lot of sense to me.

Feels like you are pretty much on the right track, yes.

TomJGooding and others added 3 commits August 31, 2023 17:14
@TomJGooding @rodrigogiraoserrao
add star to init
c0a062b 
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
@TomJGooding
remove unnecessary validations
ea5213a
@TomJGooding
update rule styles
4eb596c
@TomJGooding
Copy link
Copy Markdown
Collaborator Author

TomJGooding commented Aug 31, 2023 •
edited
Loading

I didn't think of using the border edge styles as styles for the rule but that makes a lot of sense to me.

Sorry, I've now changed my mind on this! I realised many border styles don't actually make sense in this context (e.g., round, panel, tall, hkey), so I've reduced the rule styles accordingly. See updated screenshots above.

@TomJGooding
Copy link
Copy Markdown
Collaborator Author

TomJGooding commented Sep 3, 2023

Sorry @rodrigogiraoserrao a quick question: do I need to make the non-widget Rule classes available, i.e. RuleOrientation, LineStyle, and custom exceptions?

@rodrigogiraoserrao
Copy link
Copy Markdown
Contributor

rodrigogiraoserrao commented Sep 4, 2023

Sorry @rodrigogiraoserrao a quick question: do I need to make the non-widget Rule classes available, i.e. RuleOrientation, LineStyle, and custom exceptions?

I'd say so, yes.
Maybe you saw them already, but take a look at src/textual/widgets/button.py, src/textual/widgets/data_table.py, and the other *.py files inside widgets that don't start with an underscore.

@TomJGooding TomJGooding marked this pull request as ready for review September 4, 2023 12:04
@TomJGooding
Copy link
Copy Markdown
Collaborator Author

TomJGooding commented Sep 4, 2023

Thanks for confirming. I've marked this as ready for review, but I appreciate there is a lot else happening in Textual at the moment!

rodrigogiraoserrao
rodrigogiraoserrao reviewed Sep 4, 2023
View reviewed changes
willmcgugan
willmcgugan approved these changes Sep 4, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, Tom!

@TomJGooding
Copy link
Copy Markdown
Collaborator Author

TomJGooding commented Sep 4, 2023

Haha, looks like I just mistimed merging main so I could update the changelog! Thanks Will.

@willmcgugan willmcgugan merged commit 06b6426 into Textualize:main Sep 4, 2023
@TomJGooding TomJGooding deleted the feat-add-rule-widget branch September 4, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@willmcgugan willmcgugan willmcgugan approved these changes

+1 more reviewer

@rodrigogiraoserrao rodrigogiraoserrao rodrigogiraoserrao left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Rule widget

3 participants

@TomJGooding @rodrigogiraoserrao @willmcgugan