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

add next/image and next-video in next namespace #2223

Merged
merged 7 commits into from Dec 7, 2023

Conversation

Lendemor
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

i think we should keep rx.image pointing to the Chakra image for now, but we should get this out there and switch reflex-web over to use it and do some more battle testing.

@Lendemor Lendemor changed the title add next/image as default rx.image add next/image and next-video in next namespace Dec 1, 2023
@Lendemor Lendemor requested a review from masenf December 1, 2023 12:57
Copy link
Collaborator

@masenf masenf 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 image stuff works and serialization of PIL images is still working.

i'm not sure about next-video, but it doesn't seem to be harming anything here, just not sure if it can actually be used.

Comment on lines +11 to +16
class Video(NextComponent):
"""A video component from NextJS."""

tag = "Video"
library = "next-video"
is_default = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this component actually do? reading through the docs here https://next-video.dev/docs

it seems like it's a video uploading and hosting service with nextjs integration. i'm not 100% sure this makes sense in reflex, especially not without the associated next.config.js

/** @type {import('next').NextConfig} */
const { withNextVideo } = require('next-video/process');
 
const nextConfig = {}; // Your current Next Config object
 
module.exports = withNextVideo(nextConfig);

I think this could be part of our asset story, particularly with the hosting service which does not have a lot of disk, but it seems to be missing parts and definitely needs documentation.

@picklelo picklelo merged commit caf3260 into main Dec 7, 2023
45 checks passed
@picklelo picklelo deleted the lendemor/use_next_media_components branch January 30, 2024 03:10
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.

None yet

3 participants