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

Enabled Fallback for Images #3844

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

abdulhakkeempa
Copy link

@abdulhakkeempa abdulhakkeempa commented Aug 27, 2024

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

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?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

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

a. Enabled Fallback for Images
b.  This pull request enables the “fallback” for rx.image(). The src parameter can accept local file paths, web URLs, or Pillow image objects as per the documentation of Reflex. When the specified image is unavailable or the URL is invalid, the fallback URL (if provided) is used as an alternative. The fallback URL undergoes the same checks as the original src. Currently, this feature is implemented in the base HTML image section, with the possibility of extending it to the next/image component.

closes #3838

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.

Thanks for the contribution! I think to handle this in the generic case we may need to do something like this: https://stackoverflow.com/questions/66949606/what-is-the-best-way-to-have-a-fallback-image-in-nextjs and set the onError prop. This will allow the error handling to occur on the frontend during runtime rather than during compile time.

@@ -132,6 +138,60 @@ def create(cls, *children, **props) -> Component:
The component.

"""

def validate_image(src):
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good approach, but this will only be able to validate images that are hard-coded during compile time. If they do something like rx.image(src=State.image) and bind it to a var, the validation won't happen. Not sure there's anything we can do for that general case though...

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @picklelo for the review. So your suggestion is to do the fallback feature using the onError property. Since this property is directly related with Next.js is that functionality to be implemented here reflex/components/el/elements/media.py or reflex/components/next/image.py.

Copy link
Contributor

Choose a reason for hiding this comment

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

@abdulhakkeempa Yes the onError will allow the check to happen on the client's browser. I think we need to put it in the media.py since that's what rx.image maps to.

This may be a bit tricky actually, there's a few steps involved. I think the first step is to add the on_error event handler to the Image (it should work within the base image as well: https://stackoverflow.com/questions/34097560/react-js-replace-img-src-onerror). Then for now we ca have the user set it to an event handler in the state that updates the image src.

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.Image should have a fallback prop
2 participants