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.

Sign up for GitHub

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

Jump to bottom

Create Checklist and RadioItems labels with titles #3068

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Create Checklist and RadioItems labels with titles #3068

wants to merge 1 commit into from

Conversation

giladmaya
Copy link

@giladmaya giladmaya commented Nov 8, 2024 •
edited
Loading

This PR fixes #1847 and adds a title to the Checklist and RadioItems options

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Propagate title to label
    • Implement integration tests
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@giladmaya giladmaya force-pushed the add-title-to-checklist-and-radioitem branch 2 times, most recently from 5b5a6ca to d5a97fb Compare November 8, 2024 22:18
@giladmaya giladmaya marked this pull request as ready for review November 9, 2024 10:28
@gvwilson gvwilson added feature something new P1 needed for current cycle community community contribution labels Nov 11, 2024
T4rk1n
T4rk1n reviewed Nov 13, 2024
View reviewed changes
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

Looks good, just a few non blocking comments on the test refactor and missing a changelog entry.

...omponents/tests/integration/dropdown/test_dropdown_radioitems_checklist_option_title_prop.py Outdated Show resolved Hide resolved
...omponents/tests/integration/dropdown/test_dropdown_radioitems_checklist_option_title_prop.py Outdated Show resolved Hide resolved
...omponents/tests/integration/dropdown/test_dropdown_radioitems_checklist_option_title_prop.py Outdated Show resolved Hide resolved
@giladmaya giladmaya force-pushed the add-title-to-checklist-and-radioitem branch 5 times, most recently from 46e2d0c to 14c8a23 Compare November 21, 2024 05:47
@giladmaya
Copy link
Author

Looks good, just a few non blocking comments on the test refactor and missing a changelog entry.

I've addressed all your comments and pushed the changes.
The tests fail as a result of an issue (unrelated to my suggested changes) with Google Chrome installation.

Can you please re-review? Thank you

@giladmaya giladmaya requested a review from T4rk1n November 22, 2024 07:17
T4rk1n
T4rk1n approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

💃 Looks good 🎉

The failing test is unrelated, I'll fix it.

@giladmaya giladmaya force-pushed the add-title-to-checklist-and-radioitem branch from b6a725a to ff244d3 Compare November 23, 2024 14:51
@giladmaya
Copy link
Author

💃 Looks good 🎉

The failing test is unrelated, I'll fix it.

@T4rk1n I've tried re-triggering the failed tests but it seems like there's some instability with them, which as far as I can tell isn't related to my code changes. Can you please have a look? Thank you

@T4rk1n
Copy link
Contributor

T4rk1n commented Nov 25, 2024

💃 Looks good 🎉
The failing test is unrelated, I'll fix it.

@T4rk1n I've tried re-triggering the failed tests but it seems like there's some instability with them, which as far as I can tell isn't related to my code changes. Can you please have a look? Thank you

We're just hitting some bad luck with the flaky tests, no worry I'll make them pass and merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@T4rk1n T4rk1n T4rk1n approved these changes

@alexcjohnson alexcjohnson Awaiting requested review from alexcjohnson alexcjohnson is a code owner

@ndrezn ndrezn Awaiting requested review from ndrezn ndrezn is a code owner

@gvwilson gvwilson Awaiting requested review from gvwilson gvwilson is a code owner

@emilykl emilykl Awaiting requested review from emilykl emilykl is a code owner

Assignees
No one assigned
Labels
community community contribution feature something new P1 needed for current cycle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow setting the title property of the label surrounding the RadioItem and Checklist choices.
3 participants
@giladmaya @T4rk1n @gvwilson