Skip to content

Conversation

@bartoszherba
Copy link
Collaborator

@bartoszherba bartoszherba commented May 18, 2022

Description

Currently, we are handling logic for any type of product directly inside the page component which is messy, hard to maintain, modify and understand. Because each product in Magento has its own features, we should add dedicated renderers with dedicated logic

refactor: create renderers for each product type
- split root product page into a separate components
- move styles to shared file
- reduce the amount of unused code in different component types
- add useGallery composable
- add useProductTabs composable
- add canAddToCart method on useCart composable

Related Issue

#804

Motivation and Context

Improvement

How Has This Been Tested?

Screenshots (if appropriate):

Screenshot 2022-05-20 at 11 38 40

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@bartoszherba bartoszherba force-pushed the M2-364-create-renderers-for-each-product-type branch 2 times, most recently from 858a7ee to e740db5 Compare May 19, 2022 12:33
Copy link
Contributor

@sethidden sethidden left a comment

Choose a reason for hiding this comment

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

Given you've mentioned you still want to work on this in future iterations, don't you want to make a separate feature branch? I'm reluctant to merge this to develop directly because of all the duplication in template duplication between product variants

(not requesting changes or approving for now)

},
props: {
product: {
type: [Object, null] as PropType<Product>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type: [Object, null] as PropType<Product>,
type: [Object, null] as PropType<Product | null>,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

import { useImage } from '~/composables';
import { Product } from '~/modules/catalog/product/types';

export function useGallery(product: Product) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function useGallery(product: Product) {
export function useGallery(product: Ref<Product>) {

Is this reactive? I remember when I was creating useProductsWithCommonCardProps.ts I needed to pass a ref through the argument, not a regular value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand ;) this is MY NEW composable ;) Param is not a ref but the output is a computed value (at least partially).

@@ -0,0 +1,189 @@
.product {
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is going to be broken down later but it's pretty scary that all the product variant components use the same styles. Like, if I change one thing I'd be scared of the collateral damage leaking into other components.

But as I said, I assume you're going to be extracting certain shared things to separate components

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you suggest that I should move it back to components? Or maybe add style file for each group?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you suggest that I should move it back to components?

I mean move it to even more deeply nested components, so the component tree would look like

product.vue > BundledProduct.vue > ProductReviews.vue
product.vue > ConfigurableProduct.vue > ProductReviews.vue

etc.

for more strict encapsulation in a component instead of sharing styles

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will create a task for that too.

@bartoszherba bartoszherba force-pushed the M2-364-create-renderers-for-each-product-type branch 2 times, most recently from 7c05a84 to af40762 Compare May 20, 2022 08:32
@bartoszherba bartoszherba requested a review from sethidden May 20, 2022 08:39
@bartoszherba bartoszherba marked this pull request as ready for review May 20, 2022 08:40
@bartoszherba bartoszherba requested a review from Frodigo as a code owner May 20, 2022 08:40
sethidden
sethidden previously approved these changes May 20, 2022
- split root product page into a separate components
- move styles to shared file
- reduce the amount of unused code in different component types
- add useGallery composable
- add useProductTabs composable
- add canAddToCart method on useCart composable
@bartoszherba bartoszherba force-pushed the M2-364-create-renderers-for-each-product-type branch from af40762 to 9080c1e Compare May 20, 2022 09:36
@bartoszherba bartoszherba requested a review from sethidden May 20, 2022 09:36
@bartoszherba bartoszherba merged commit de161f1 into develop May 20, 2022
@bartoszherba bartoszherba deleted the M2-364-create-renderers-for-each-product-type branch May 20, 2022 09:46
@Frodigo Frodigo added this to the 1.0.0-rc.8 milestone May 25, 2022
@Frodigo Frodigo changed the title refactor: create renderers for each product type refactor!: create renderers for each product type May 25, 2022
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.

4 participants