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: cocktail card - save to image #7

Merged
merged 28 commits into from
Jan 22, 2024
Merged

Conversation

mildrrnt
Copy link
Contributor

Change made

  • Bug fixes
  • New features
  • Breaking changes

Describe what you have done

  • Add 6 pages for cocktail cards with save to image button, allowing users to capture result without button components on screen.

New Features

  • Save to image

@mildrrnt mildrrnt requested a review from shalluv January 10, 2024 14:12
@shalluv
Copy link
Member

shalluv commented Jan 10, 2024

You can run pnpm format to format all files.

return (
<div
id="buttonSet"
className="relative mx-auto my-auto mt-2 flex w-[16rem] gap-4"
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid arbitrary value

Suggested change
className="relative mx-auto my-auto mt-2 flex w-[16rem] gap-4"
className="relative mx-auto my-auto mt-2 flex w-64 gap-4"

@mildrrnt
Copy link
Contributor Author

You can run pnpm format to format all files. @shalluv

Do you have any suggestion on formatting? I think what cause the issue is the text inside

Before running pnpm format, it looks like this:

image

However, after running pnpm format, it looks like this and affect the formatting of the actual page.

image

@mildrrnt mildrrnt changed the title Cocktail Card - Save to Image feat: cocktail card - save to iamge Jan 10, 2024
@mildrrnt mildrrnt changed the title feat: cocktail card - save to iamge feat: cocktail card - save to image Jan 10, 2024
@mildrrnt mildrrnt self-assigned this Jan 10, 2024
@shalluv
Copy link
Member

shalluv commented Jan 11, 2024

It's actually not a great idea to separate lines like that. Please use <br /> tag and everything will be fine!

@mildrrnt
Copy link
Contributor Author

Thank you for your comment kub. Sorry for my frequent commits and pushes, I run pnpm format twice/thrice for it to pass.

Another thing is that I just realized that I could run pnpm lint locally...

<div className="absolute inset-x-0 -left-[1.25rem] bottom-32 opacity-80">
<img
className="absolute left-0 top-[48px] h-32 w-[152px]"
src="/src/images/elements/calendar.png"
Copy link
Member

Choose a reason for hiding this comment

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

Please import it first as

import Calendar from "@/assets/images/elements/calendar.png"

// then

<img ... src={Calendar.src} ... />

@shalluv
Copy link
Member

shalluv commented Jan 16, 2024

Since Card pages have the same layout, is it possible to create a shared layout or shared components for them?

@shalluv
Copy link
Member

shalluv commented Jan 16, 2024

Furthermore, the assets are already located in the src/assets directory on the dev branch. You can merge (git pull origin dev) the dev branch into this one to access them. Please remove any redundant images from the src/images directory.

Copy link
Member

@shalluv shalluv left a comment

Choose a reason for hiding this comment

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

Great job!

@shalluv shalluv merged commit c671517 into dev Jan 22, 2024
1 check failed
@shalluv shalluv deleted the worranittha/gea-18-save-to-image branch January 22, 2024 11:59
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.

2 participants