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

Best practices first version #1455

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Best practices first version #1455

wants to merge 7 commits into from

Conversation

dydome
Copy link
Contributor

@dydome dydome commented Apr 25, 2022

close #1454

Copy link
Contributor

@szczepaniak-michal szczepaniak-michal left a comment

Choose a reason for hiding this comment

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

Are you sure that you want to commit .DS_Store files? Seems as if they are some macOS files that should not be here

Copy link

@mateuszo mateuszo left a comment

Choose a reason for hiding this comment

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

Good job ;)

})
export class CustomCartTotalsComponent extends CartTotalsComponent implements OnInit {

constructor(

Choose a reason for hiding this comment

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

If we're not changing the dependencies of the component, we can omit constructor overriding because it would be inherited. Omitting the constructor, in this case, has the benefit that after the Spartacus version upgrade we don't need to do any modifications to our custom code.

At the same time it would be valuable to add an additional example where we do need to add an additional dependency to our component.

},
]
})
export class CartTotalModule { }

Choose a reason for hiding this comment

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

This is to be mentioned somewhere else but I always suggest to add a custom prefix to all the elements. Here it is easy to mistake custom CartTotalsModule with the core one.

},
]
})
export class CartTotalModule { }

Choose a reason for hiding this comment

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

This is to be mentioned somewhere else but I always suggest to add a custom prefix to all the elements. Here it is easy to mistake custom CartTotalsModule with the core one.

`i18n.ts` example:

```
export const customi18nConfig: I18nConfig = {

Choose a reason for hiding this comment

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

I think that customI18nConfig is more readable

3. Copy Spartacus Storefron Component template and paste it in our own Storefront template with our own customization. We can here:
- modify elements order
- add custom elements
4. Import in our Storefront Module all required modules

Choose a reason for hiding this comment

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

For some reason "our Storefront Module" sounds strange to me. Maybe because you're writing this as a core team member and in this case, "our" may refer to the core code ;).
I would write either:

  • your Storefront Module, or
  • our custom Storefront Module

```export const customLayoutConfig: LayoutConfig = {
layoutSlots: {
prologue: {
xl: {

Choose a reason for hiding this comment

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

I don't know if you want to add this but I personally don't recommend using breakpoints in the layout config, because it affects performance and causes flickering in SSR. Responsiveness should be achieved via CSS whenever it's possible.


### Extending pageMetaResolvers and normalizers

Extending Page Meta Resolvers or Normalizer looks the same as with adapters, the only difference is providing them.

Choose a reason for hiding this comment

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

I think it would be worthwhile to mention why we need to add it as multi-provider and how resolvers are being selected (getScore() method) - this is usually difficult to understand for the newcomers.


Under `app`, create `shared` folder. This folder will contain all elements used globally in the project like cms-components, components, adapters, connectors, guards, configs, directives, pipes, etc.

We will suggest creating a separate folder for each of the above examples. We will recommend also dividing the folder for components into: `components` and `cms-components`, the first one is to keep shared components, but the second one is to keep all shared components with cms mapping.

Choose a reason for hiding this comment

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

I guess this will undergo proofreading but using "will" sounds bad to me here. It sounds like you don't suggest/recommend something yet. I think it should be:

  • "we suggest"
  • "we recommend"


### Features folder

Next folder we will recommand to create is `features` folder next to the `shared`. This folder will containt all page main features/modules. For each page funcionality/module we should create seperate folder, including following folders:

Choose a reason for hiding this comment

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

same as above


Under `app`, create `shared` folder. This folder will contain all elements used globally in the project like cms-components, components, adapters, connectors, guards, configs, directives, pipes, etc.

We will suggest creating a separate folder for each of the above examples. We will recommend also dividing the folder for components into: `components` and `cms-components`, the first one is to keep shared components, but the second one is to keep all shared components with cms mapping.

Choose a reason for hiding this comment

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

I'm not sure whether cms components should go into a shared folder. They are usually feature specific and they are used only once in the source code - in the mapping configuration. In my opinion putting them into shared folder might result in accidentally importing the mapping multiple times.

Copy link
Contributor Author

@dydome dydome Apr 29, 2022

Choose a reason for hiding this comment

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

@mateuszo It is only for shared elements. So if somebody has for example cms-component which is used under the cart, it will be under the cart feature module folder. But if it is banner-component we can put it in shared/cms-components/banner...

Sounds better for you?

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.

Best Practices doc
3 participants