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 booking card component #46

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

Conversation

shubhsk88
Copy link

This PR address the implementation of the booking card component in the shopping section

Copy link
Owner

@Charlie85270 Charlie85270 left a comment

Choose a reason for hiding this comment

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

Hi shubhsk88 and thanks for your PR !

I've added a few comments, but you couldn't have known. I will add more documentation about adding components (rules to follow, good practice, etc ...). I will add linter rules too.

Don't forget to increment the counter of the component's category, here on file: /components/kit/components/commerce/index.tsx

Don't hesitate to contact me if you have any questions ;)

Thanks,

Charlie

const BookingCard: FC = () => {
return (
<div className="lg:flex max-w-2xl mx-auto overflow-hidden rounded-lg m-24 shadow-lg">
<img
Copy link
Owner

Choose a reason for hiding this comment

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

Images must be serve locally, please download this image on the project.
Then the image must be resized, compressed and optimised so that it is as light as possible (here around 30 ko).
You can do it with this tools : https://ezgif.com/resize for example.

Copy link
Author

Choose a reason for hiding this comment

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

Can't you wrap that in the next image component if possible usually from version 10 I tend to use that in my projects

package.json Outdated
@@ -33,5 +33,8 @@
"react-dom": "^17.0.1",
"react-live": "^2.2.3",
"tailwindcss": "^2.0.1"
},
"volta": {
Copy link
Owner

Choose a reason for hiding this comment

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

Volta is not useful here


const BookingCard: FC = () => {
return (
<div className="lg:flex max-w-2xl mx-auto overflow-hidden rounded-lg m-24 shadow-lg">
Copy link
Owner

Choose a reason for hiding this comment

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

Can you implement the dark version of the component ? ( see : https://tailwindcss.com/docs/dark-mode)

@Charlie85270 Charlie85270 added the enhancement New feature or request label Jan 26, 2021
Copy link
Owner

@Charlie85270 Charlie85270 left a comment

Choose a reason for hiding this comment

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

yes I will add Next/image for optimisation to the project and notify you when it's deploy ;)


const BookingCard: FC = () => {
return (
<div className="lg:flex max-w-2xl mx-auto overflow-hidden rounded-lg m-24 shadow-lg">
Copy link
Owner

Choose a reason for hiding this comment

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

m-24 must to be added on container, not on component.
Component's code must ready to paste and not provide "margin" around it

@shubhsk88
Copy link
Author

shubhsk88 commented Jan 30, 2021

I haven't heard your feedback @Charlie85270 . Please let me know

@Charlie85270
Copy link
Owner

Hi shubhsk88,

Can you resolve conflicts please ?
Without activity in 2 days I would be forced to close your PR

Thanks,

Charlie

@shubhsk88
Copy link
Author

It is only conflicting the new file import added I think this won't be a problem

@shubhsk88
Copy link
Author

Is there any maintainer that is being helpful here? Or people just keep ignoring other person request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants