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

ability to pass a component in light prop #1405

Merged
merged 3 commits into from
Jul 17, 2022

Conversation

idiglove
Copy link
Contributor

@idiglove idiglove commented Feb 24, 2022

the light prop could be like this:
light={<img style={{position: 'relative', width: '100%', height: '100%'}} src='https://media.nationalgeographic.org/assets/photos/000/290/29094.jpg' alt='sun' />}

The styling of the image component should come from where it was called

The play icon will be positioned absolute if light is a component

image

refers to #1320

Copy link

@n3llym n3llym left a comment

Choose a reason for hiding this comment

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

Thanks @idiglove, and good job. It would be great to get this added!

Just a few things I noticed I've added some inline comments as well as the below.

  1. You need to add the new type in base.d.ts i.e light?: boolean | string | ReactElement
  2. You need to update the light prop type in props.js i.e light: oneOfType([bool, string, object])
  3. Also don’t forget to run lint and run the tests!

src/Preview.js Outdated Show resolved Hide resolved
src/Preview.js Show resolved Hide resolved
src/Preview.js Show resolved Hide resolved
@idiglove
Copy link
Contributor Author

idiglove commented Jun 9, 2022

Thanks @n3llym I've done the changes, linted and tested. Appreciate your review!

@cookpete cookpete force-pushed the feature/preview-as-component branch from cba4125 to f386d87 Compare July 17, 2022 12:42
@cookpete
Copy link
Owner

I tidied things up a bit to remove the need for the extra stuff in state. Nice work!

@cookpete cookpete merged commit 265d5e0 into cookpete:master Jul 17, 2022
@pokey pokey mentioned this pull request Aug 11, 2022
@cookpete
Copy link
Owner

Published in 2.11.0. Sorry for the delay.

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