Skip to content

ability to pass a component in light prop - #1405

Merged
cookpete merged 3 commits into
cookpete:masterfrom
idiglove:feature/preview-as-component
Jul 17, 2022
Merged

ability to pass a component in light prop#1405
cookpete merged 3 commits into
cookpete:masterfrom
idiglove:feature/preview-as-component

Conversation

@idiglove

@idiglove idiglove commented Feb 24, 2022

Copy link
Copy Markdown
Contributor

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

@n3llym n3llym left a comment

Copy link
Copy Markdown

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!

Comment thread src/Preview.js Outdated
Comment thread src/Preview.js
Comment thread src/Preview.js
@idiglove

idiglove commented Jun 9, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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.

3 participants