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

Bug fix rx.toast. Allow passing title kwarg #3857

Merged
merged 1 commit into from
Aug 31, 2024
Merged

Conversation

TimChild
Copy link
Contributor

@TimChild TimChild commented Aug 29, 2024

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
    Yes

  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?
    Yes

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them? Yes
  • Have you written new tests for your core changes, as applicable?
    No -- Did not find any existing tests for toasts, and wasn't sure where to put them.
  • Have you successfully ran tests with your changes locally? Current tests all pass

After these steps, you're ready to open a pull request.

a. Give a descriptive title to your PR. -- Done

b. Describe your changes.

  • Added title to the ToastProps(PropsBase) so that a validation error isn't raised if title is passed explicitly
  • Made message defualt to an empty string for toast.info, toast.error, etc. -- Note: A error is already raised in Toaster.send_toast if message is empty and title/description are not provided

c. Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if such).

Note: I tested manually (in an example app) with these examples:

def _toasts() -> list:
    return [
        rx.toast("toast('message', level='info')", level="info"),
        rx.toast.info("toast.info('message')"),
        rx.toast.info(message="toast.info(message='message')"),
        rx.toast.info(
            "toast.info('title', description='description')", description="description"),
        rx.toast.info(
            title="toast.info(title='title', description='description')", description="description"),
        rx.toast.info(
            message='message', title="toast.info(message='message', title='title', description='description')", description="description"),  # message is ignored in this case
    ]

I did not find an obvious place to add real unit tests for this. I'm also not sure how I would go about that outside of AppHarness tests.

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

awesome, thanks for adding!

@picklelo picklelo merged commit 99a0236 into reflex-dev:main Aug 31, 2024
46 checks passed
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.

rx.Toast raises validation error if title prop passed explicitly in 0.5.10
2 participants