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

feat(card): support custom renderImage functions for Card #730

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

levino
Copy link
Contributor

@levino levino commented May 3, 2023

Since in some cases, for example next.js apps, one needs to use a specific and optimized Image component, the Card component now accepts a renderImage property to render your own component. If renderImage is set, imgSrc and imgAlt are ignored. TypeScript will error if the user attempts to set them at the same time.

fix #706

To do:

  • Add a test with falsy imgSrc -> should not render the an image tag
  • Update documentation.

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage: 99.77% and project coverage change: -0.02 ⚠️

Comparison is base (7461173) 99.54% compared to head (b6a5c87) 99.53%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
- Coverage   99.54%   99.53%   -0.02%     
==========================================
  Files         163      166       +3     
  Lines        6621     6865     +244     
  Branches      401      420      +19     
==========================================
+ Hits         6591     6833     +242     
- Misses         30       32       +2     
Impacted Files Coverage Δ
src/components/Dropdown/Dropdown.tsx 99.22% <99.41%> (-0.78%) ⬇️
src/components/Button/Button.tsx 100.00% <100.00%> (ø)
src/components/Button/ButtonBase.tsx 100.00% <100.00%> (ø)
src/components/Button/theme.ts 100.00% <100.00%> (ø)
src/components/Card/Card.tsx 100.00% <100.00%> (ø)
src/components/Carousel/Carousel.tsx 99.02% <100.00%> (+0.06%) ⬆️
src/components/DarkThemeToggle/DarkThemeToggle.tsx 100.00% <100.00%> (ø)
src/components/Dropdown/DropdownItem.tsx 100.00% <100.00%> (ø)
src/components/Dropdown/theme.ts 100.00% <100.00%> (ø)
src/components/Floating/Floating.tsx 100.00% <100.00%> (ø)
... and 8 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor Author

@levino levino left a comment

Choose a reason for hiding this comment

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

This is a first draft. Please have a look and let me know what you like and what not.

src/lib/components/Card/Card.stories.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.tsx Outdated Show resolved Hide resolved
src/lib/helpers/omit.ts Outdated Show resolved Hide resolved
@levino
Copy link
Contributor Author

levino commented May 4, 2023

I will continue to work on this later. I will also update the docs.

src/lib/helpers/omit.ts Outdated Show resolved Hide resolved
src/lib/helpers/omit.ts Outdated Show resolved Hide resolved
src/lib/components/Card/Card.spec.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.stories.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.stories.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.tsx Outdated Show resolved Hide resolved
@levino
Copy link
Contributor Author

levino commented May 9, 2023

Can someone please review my PR? Would be so sad if all the work was in vain...

Copy link
Collaborator

@rluders rluders left a comment

Choose a reason for hiding this comment

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

Looks great. I requested a few changes, and I would also love to listen to @tulup-conner's opinion on this one.

src/lib/components/Card/Card.stories.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.tsx Outdated Show resolved Hide resolved
src/lib/components/Card/Card.tsx Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
@levino
Copy link
Contributor Author

levino commented May 10, 2023

Just FYI: The docs have become somewhat ugly (at least storybook / docs):
Screenshot 2023-05-10 at 11 01 42

@levino
Copy link
Contributor Author

levino commented May 10, 2023

Once this has been accepted, I will rebase it all into one clean commit.

src/lib/components/Card/Card.tsx Outdated Show resolved Hide resolved
@levino
Copy link
Contributor Author

levino commented May 15, 2023

@rluders Could you please resolve all conversations that have been resolved from your point of view? Will @tulup-conner come by any time soon to give their final review?

@rluders
Copy link
Collaborator

rluders commented May 25, 2023

@levino I think that this one is good, if you could just fix the test - it is good to be merged then.

@vercel
Copy link

vercel bot commented May 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flowbite-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 12, 2023 6:16am

@levino
Copy link
Contributor Author

levino commented May 26, 2023

I still need to update the "real" documentation with an example. Putting this to draft until I have time to fix that, probably next week. Enjoy the week end and thank you for coming back to this @rluders

@levino levino marked this pull request as draft May 26, 2023 07:50
@rluders
Copy link
Collaborator

rluders commented May 29, 2023

@levino I think that this is a problem that we are having right now with the CodePreview, we are trying to fix it. So, I would say... maybe doesn't matter right now?

@rluders rluders mentioned this pull request May 29, 2023
15 tasks
@cglacet
Copy link

cglacet commented Jul 6, 2023

Any news on this?

@levino
Copy link
Contributor Author

levino commented Jul 6, 2023

Sorry for dropping the ball. Will try to come back to this asap. Ping me please, if I forget.

@levino levino marked this pull request as ready for review July 6, 2023 20:44
@levino levino requested a review from rluders July 6, 2023 20:51
Copy link
Contributor Author

@levino levino left a comment

Choose a reason for hiding this comment

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

Good to go imo.

app/docs/components/card/card.mdx Show resolved Hide resolved
src/components/Card/Card.tsx Show resolved Hide resolved
    Since in some cases, for example next.js apps, one needs to use a specific
    and optimized `Image` component, the `Card` component now accepts a
    `renderImage` property to render your own component. If `renderImage` is set,
    `imgSrc` and `imgAlt` are ignored. TypeScript will error if the user attempts
    to set them at the same time.

    fix themesberg#706
Copy link
Contributor Author

@levino levino left a comment

Choose a reason for hiding this comment

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

@rluders I fixed the outstanding issues. If you think they are resolved, please resolve the conversations. Other than that, I would say, this can go out. Enjoy!

app/docs/components/card/card.mdx Show resolved Hide resolved
src/components/Card/Card.tsx Show resolved Hide resolved
@rluders
Copy link
Collaborator

rluders commented Jul 12, 2023

Amazing job, @levino. Thank you very much.

@rluders rluders changed the title feat: support custom renderImage functions for Card feat(card): support custom renderImage functions for Card Jul 12, 2023
@rluders rluders merged commit 594f187 into themesberg:main Jul 12, 2023
6 checks passed
rluders pushed a commit to rluders/flowbite-react that referenced this pull request Jul 16, 2023
…#730)

feat: support custom renderImage functions for Card

    Since in some cases, for example next.js apps, one needs to use a specific
    and optimized `Image` component, the `Card` component now accepts a
    `renderImage` property to render your own component. If `renderImage` is set,
    `imgSrc` and `imgAlt` are ignored. TypeScript will error if the user attempts
    to set them at the same time.

    fix themesberg#706
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.

Use NextJS image optimization
5 participants