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

Consolidate ResourceCards and implement Storybook POC #3784

Merged
merged 25 commits into from
Jun 21, 2023
Merged

Consolidate ResourceCards and implement Storybook POC #3784

merged 25 commits into from
Jun 21, 2023

Conversation

lilbitner
Copy link
Contributor

@lilbitner lilbitner commented Jun 15, 2023 •
edited
Loading

Purpose

  1. This PR refactors the two existing ResourceCards into a more reusable ProductCard component (s/o to Lisa for a lot of this original refactor!).
  2. This PR also adds Storybook to the Apps repository (the original POC of Storybook) to help facilitate development of components and to try the workflow of having it within the Apps repository as a team. Take a look at this quick overview document of thoughts and approaches to implementing Storybook within our workflow for this project.

Approach

With the refactor, many components were broken out into smaller components, as you will find in the ProductCard directory under the Common directory. Additionally, the new components were placed within the respective parent components to actively render in the application.

For the storybook implementation, a ProductCardStories directory is placed in this same ^^ directory, where you will find two different story files to render 'Field' type ProductCards and 'Dialog' type ProductCards. To see these stories, make sure npm i and npm run storybook. I added several iterations, but this is very much a layout I would appreciate any and all feedback on. There is also argument to add stories for the smaller subcomponents (ProductCardHeader etc) but limited the scope to just ProductCard for now to see how we like it.

Known issues

  1. The prettier build failures, looking into 👍

Testing steps

Pull down this branch and make sure everything looks as expected within the app. 🤞 There may be a few details I missed for the field vs dialog location of the card, so please let me know if you notice anything!

@lilbitner
Copy link
Contributor Author

lilbitner commented Jun 16, 2023 •
edited
Loading

I notice there is some prettier failures, but I am a bit confused by the log from the build 🤔 looking into it!

@lilbitner lilbitner marked this pull request as ready for review June 16, 2023 15:11
@lilbitner lilbitner requested a review from a team as a code owner June 16, 2023 15:11
@lilbitner lilbitner changed the title Feat/integ 673 675 Feat/integ Consolidate ResourceCards and implement Storybook POC Jun 16, 2023
@lilbitner lilbitner changed the title Feat/integ Consolidate ResourceCards and implement Storybook POC Consolidate ResourceCards and implement Storybook POC Jun 16, 2023
whitelisab
whitelisab approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@whitelisab whitelisab left a comment

Choose a reason for hiding this comment

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

Awesome work here! ⭐ Nothing truly blocking, just some comments/questions for you. You can decide whether we fix up the badge in this PR or in a follow up.

apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCard/ProductCard.tsx Outdated Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCard/ProductCard.tsx Outdated Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCard/ProductCard.tsx Outdated Show resolved Hide resolved
...c/components/Common/ProductCard/ProductCard/ProductCardStories/DialogProductCard.stories.tsx Outdated Show resolved Hide resolved
...ecommerce/frontend/src/components/Common/ProductCard/ProductCard/ProductCardStories/mocks.ts Outdated Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCardBadge/ProductCardBadge.tsx Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCardBadge/ProductCardBadge.tsx Outdated Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCardBody/ProductCardBody.tsx Show resolved Hide resolved
apps/ecommerce/frontend/src/hooks/field/useResourceMenuItems.tsx Outdated Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCardBody/ProductCardBody.tsx Outdated Show resolved Hide resolved
@lilbitner lilbitner force-pushed the feat/INTEG-673-675 branch from 4a0018d to be807a5 Compare June 20, 2023 14:36
ryunsong-contentful
ryunsong-contentful reviewed Jun 20, 2023
View reviewed changes
Copy link
Contributor

@ryunsong-contentful ryunsong-contentful left a comment

Choose a reason for hiding this comment

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

Nice! Two main themes in the review

  1. Algo improvement opportunity
  2. Design level of base components and Product Card components

apps/ecommerce/frontend/src/components/App/Dialog/ResourceList/ResourceList.tsx Outdated Show resolved Hide resolved
apps/ecommerce/frontend/src/components/App/Field/MultipleResources/MultipleResources.tsx Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCardBadge/ProductCardBadge.tsx Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCardBadge/ProductCardBadge.tsx Show resolved Hide resolved
apps/ecommerce/frontend/src/components/Common/ProductCard/ProductCardBody/ProductCardBody.tsx Show resolved Hide resolved
apps/ecommerce/frontend/src/providers/ResourceFieldProvider.tsx Show resolved Hide resolved
apps/ecommerce/frontend/src/types.ts Show resolved Hide resolved
ryunsong-contentful
ryunsong-contentful approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@ryunsong-contentful ryunsong-contentful left a comment

Choose a reason for hiding this comment

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

Approving because I don't think the last bit of design optimizations is a great use of your time and there's a good chance even if we optimized HOC badge component, it would be changed and refactored

@lilbitner lilbitner merged commit 548e3a3 into master Jun 21, 2023
@lilbitner lilbitner deleted the feat/INTEG-673-675 branch June 21, 2023 20:09
cemreyuksel pushed a commit that referenced this pull request Jun 22, 2023
@lilbitner @whitelisab @cemreyuksel
Consolidate ResourceCards and implement Storybook POC (#3784)
c843652 
* Add storybook to ecom frontend

* WIP create new common ResourceCard

* Add Storybook config

* Remove storybook boilerplate code

* Add ProductCard components with stories and tests

* Add new stories, components, and fixed parent components

* Merge master

* Add tests and update props

* Fix failing tests

* Re-add missing component

* Cleanup code

* Delete ResourceCards

* Fix test

* Fix another test

* Remove extraneous variables

* Small cleanup

* Remove comment

* Fix card rendering in fields

* Add a few more stories and adjust tests

* Fix badge state

* PR review adjustmetns

* Adjust mocks directories

* Add more PR review feedback

* Add prettier fixes

* Fix linting errors

---------

Co-authored-by: Lisa White <lisa.white@contentful.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whitelisab whitelisab whitelisab approved these changes

@ryunsong-contentful ryunsong-contentful ryunsong-contentful approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lilbitner @whitelisab @ryunsong-contentful