-
Notifications
You must be signed in to change notification settings - Fork 22
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
_pages/dev/best-practises/extending-best-practices/extending-best-practices.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Michal Szczepaniak <[email protected]>
…est-practices.md Co-authored-by: Michal Szczepaniak <[email protected]>
Co-authored-by: Michal Szczepaniak <[email protected]>
There was a problem hiding this 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( |
There was a problem hiding this comment.
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 { } |
There was a problem hiding this comment.
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 { } |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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: { |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
…s into feature/best-practices
close #1454