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.

Sign up for GitHub

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

Jump to bottom

feat(pie-thumbnail): DSW-2580 add basic thumbnail functionality #2138

Merged
merged 3 commits into from
Jan 14, 2025
Merged

feat(pie-thumbnail): DSW-2580 add basic thumbnail functionality #2138

merged 3 commits into from
Jan 14, 2025

Conversation

bntsv
Copy link
Contributor

@bntsv bntsv commented Jan 7, 2025 •
edited by jamieomaguire
Loading

Describe your changes (can list changeset entries if preferable)

  • Added the basic pie-thumbnail functionality - variant, src and alt props
  • Added component to storybook
  • Added visual tests

Author Checklist (complete before requesting a review, do not delete any)

  • I have performed a self-review of my code.
  • I have added thorough tests where applicable (unit / component / visual).
  • I have reviewed the PIE Storybook/PIE Docs PR preview.
  • I have reviewed visual test updates properly before approving.
  • If changes will affect consumers of the package, I have created a changeset entry.
  • If a changeset file has been created, I have tested these changes in PIE Aperture using the /test-aperture command.

Not-applicable Checklist items

Please move any Author checklist items that do not apply to this pull request here.

Testing

How do I test my changes?

Task Link
Aperture PR 🔗
NextJS 14 deployment 🔗
Nuxt 3 deployment 🔗
Vanilla deployment 🔗

Reviewer checklists (complete before approving)

Mark items as [-] N/A if not applicable.

Reviewer 1 @raoufswe

  • I have reviewed the PIE Storybook/PIE Docs PR preview.
  • I have verified that all acceptance criteria for this ticket have been completed.
  • I have reviewed the Aperture changes (if added)
  • If there are visual test updates, I have reviewed them.

Reviewer 2

  • I have reviewed the PIE Storybook/PIE Docs PR preview.
  • I have verified that all acceptance criteria for this ticket have been completed.
  • I have reviewed the Aperture changes (if added)
  • If there are visual test updates, I have reviewed them.

@bntsv bntsv requested review from a team as code owners January 7, 2025 13:08
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jan 7, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 9cf66cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
@justeattakeaway/pie-thumbnail Minor
pie-storybook Minor
@justeattakeaway/pie-webc Patch
wc-angular12 Patch
wc-nuxt2 Patch
wc-react17 Patch
wc-react18 Patch
wc-vue3 Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@bntsv bntsv force-pushed the dsw-2580-add-basic-thumbnail-functionality branch from 58dd0bd to e88a86c Compare January 7, 2025 13:16
@bntsv
Copy link
Contributor Author

bntsv commented Jan 8, 2025

/test-aperture

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

@bntsv Your snapshots have been published to npm!

Test the snapshots by updating your package.json with the newly-published versions:

Note

If you have more than one of these packages installed, we suggest using the new snapshots for all of them to help avoid version conflicts.

yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile

Then finally:

yarn install

@bntsv bntsv force-pushed the dsw-2580-add-basic-thumbnail-functionality branch from 3ee3c89 to 46438fb Compare January 9, 2025 15:27
@jamieomaguire
Copy link
Contributor

@bntsv this must be tested in pie-aperture, please reach out if you would like some assistance with this and we can set up some time to pair. You will see a link in the PR description that goes to our wiki page to give some more information on why/how to do this

@bntsv
Copy link
Contributor Author

bntsv commented Jan 10, 2025

@bntsv this must be tested in pie-aperture, please reach out if you would like some assistance with this and we can set up some time to pair. You will see a link in the PR description that goes to our wiki page to give some more information on why/how to do this

@jamieomaguire I discussed this with @raoufswe and he thinks that this is not required right now since we will be opening another PR after this one adding more props. We would then test in Pie Aperture

@raoufswe raoufswe force-pushed the dsw-2580-add-basic-thumbnail-functionality branch from d5db0a5 to 4533fde Compare January 13, 2025 17:32
@raoufswe
feat(pie-thumbnail): DSW-2580 add basic thumbnail functionality
0de40ae 
format(pie-thumbnail): DSW-2580 lint

test(pie-thumbnail): DSW-2580 add visual tests

test(pie-thumbnail): DSW-2580 update visual tests

refactor(pie-thumbnail): DSW-2580 address comments

refactor(pie-thumbnail): DSW-2580 remove placeholder image from storybook

chore(pie-thumbnail): DSW-2580 update changelog

refactor(pie-thumbnail): DSW-2580 add default src and alt props to visual test

refactor(pie-thumbnail): DSW-2580 update based on comments

feat(pie-thumbnail): DSW-2580 add basic thumbnail functionality

refactor(pie-thumbnail): DSW-2580 address comments

refactor(pie-thumbnail): DSW-2580 remove placeholder image from storybook

fix(pie-thumbnail): DSW-2580 fix percy test

fix(pie-thumbnail): DSW-2580 fix percy test

refactor(pie-thumbnail): DSW-2580 update based on comments

format(pie-thumbnail): DSW-2580 address comments

feat(pie-thumbnail): DSW-2580 add basic thumbnail functionality

test(pie-thumbnail): DSW-2580 add visual tests

test(pie-thumbnail): DSW-2580 update visual tests

refactor(pie-thumbnail): DSW-2580 address comments

refactor(pie-thumbnail): DSW-2580 remove placeholder image from storybook

feat(pie-thumbnail): DSW-2580 add basic thumbnail functionality

test(pie-thumbnail): DSW-2580 add visual tests

test(pie-thumbnail): DSW-2580 update visual tests

refactor(pie-thumbnail): DSW-2580 address comments

refactor(pie-thumbnail): DSW-2580 remove placeholder image from storybook
@raoufswe raoufswe force-pushed the dsw-2580-add-basic-thumbnail-functionality branch from 4533fde to 0de40ae Compare January 13, 2025 17:36
@raoufswe
Copy link
Member

/test-aperture

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

@raoufswe Your snapshots have been published to npm!

Test the snapshots by updating your package.json with the newly-published versions:

Note

If you have more than one of these packages installed, we suggest using the new snapshots for all of them to help avoid version conflicts.

yarn up @justeattakeaway/[email protected] --mode=update-lockfile
yarn up @justeattakeaway/[email protected] --mode=update-lockfile

Then finally:

yarn install

@jamieomaguire
Copy link
Contributor

Looks like the screenreader works, nice!

Screenshot 2025-01-14 at 11 33 50

jamieomaguire
jamieomaguire approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@jamieomaguire jamieomaguire left a comment

Choose a reason for hiding this comment

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

Approved, but would appreciate you answering my questions before merging please :)

@bntsv bntsv merged commit 7471690 into main Jan 14, 2025
28 checks passed
@bntsv bntsv deleted the dsw-2580-add-basic-thumbnail-functionality branch January 14, 2025 13:29
@pie-design-system-bot pie-design-system-bot mentioned this pull request Jan 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raoufswe raoufswe raoufswe approved these changes

@jamieomaguire jamieomaguire jamieomaguire approved these changes

Assignees
No one assigned
Labels
apps components pie-storybook pie-thumbnail
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bntsv @pie-design-system-bot @jamieomaguire @raoufswe