-
Notifications
You must be signed in to change notification settings - Fork 388
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
feat: Configurable media component #19067
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rmch91
commented
Jul 23, 2024
projects/core/src/features-config/feature-toggles/config/feature-toggles.ts
Outdated
Show resolved
Hide resolved
rmch91
commented
Jul 23, 2024
rmch91
commented
Jul 23, 2024
spartacus Run #45383
Run Properties:
|
Project |
spartacus
|
Run status |
Passed #45383
|
Run duration | 03m 55s |
Commit |
09eaa5db29 ℹ️: Merge 1af972d8dd49e08e59d5832b7d3582eaa5158053 into 6d20b3be1923cc0b44332e18c3c3...
|
Committer | Roman |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
4
|
Pending |
2
|
Skipped |
0
|
Passing |
125
|
pawelfras
reviewed
Aug 6, 2024
projects/storefrontlib/shared/components/media/media.component.html
Outdated
Show resolved
Hide resolved
pawelfras
reviewed
Oct 17, 2024
...i-dimensional/selector/components/selector/product-multi-dimensional-selector.component.html
Outdated
Show resolved
Hide resolved
...mage-zoom/product-image-zoom-product-images/product-image-zoom-product-images.component.html
Outdated
Show resolved
Hide resolved
...e-zoom/product-image-zoom-product-images/product-image-zoom-product-images.component.spec.ts
Outdated
Show resolved
Hide resolved
...roduct-image-zoom/product-image-zoom-thumbnails/product-image-zoom-thumbnails.component.html
Outdated
Show resolved
Hide resolved
...components/product-image-zoom/product-image-zoom-view/product-image-zoom-view.component.html
Outdated
Show resolved
Hide resolved
Comment on lines
641
to
663
|
||
/** | ||
* When enabled, allows to provide extended formats and media queries for <picture> element if used in MediaComponent. | ||
* | ||
* For proper work requires `pictureElementFormats` provided in media config: | ||
* ```ts | ||
* provideConfig({ | ||
* pictureElementFormats: { | ||
* mediaQueries: { | ||
* 'max-width': '767px', | ||
* ... | ||
* }, | ||
* width: 50, | ||
* height: 50, | ||
* }, | ||
* }) | ||
* ``` | ||
* | ||
* After activating this toggle, new inputs in `MediaComponent` — specifically | ||
* `width`, `height`, and `sizes` — will be passed to the template as HTML attributes. | ||
* | ||
* Toggle activates `@Input() elementType: 'img' | 'picture' = 'picture';` in `MediaComponent` | ||
*/ |
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.
Suggested change
/** | |
* When enabled, allows to provide extended formats and media queries for <picture> element if used in MediaComponent. | |
* | |
* For proper work requires `pictureElementFormats` provided in media config: | |
* ```ts | |
* provideConfig({ | |
* pictureElementFormats: { | |
* mediaQueries: { | |
* 'max-width': '767px', | |
* ... | |
* }, | |
* width: 50, | |
* height: 50, | |
* }, | |
* }) | |
* ``` | |
* | |
* After activating this toggle, new inputs in `MediaComponent` — specifically | |
* `width`, `height`, and `sizes` — will be passed to the template as HTML attributes. | |
* | |
* Toggle activates `@Input() elementType: 'img' | 'picture' = 'picture';` in `MediaComponent` | |
*/ | |
/** | |
* When enabled, allows to provide extended formats and media queries for <picture> element if used in MediaComponent. | |
* | |
* For proper work requires `pictureElementFormats` provided in media config: | |
* ```ts | |
* provideConfig({ | |
* pictureElementFormats: { | |
* mediaQueries: { | |
* 'max-width': '767px', | |
* ... | |
* }, | |
* width: 50, | |
* height: 50, | |
* }, | |
* }) | |
* ``` | |
* | |
* After activating this toggle, new inputs in `MediaComponent` — specifically | |
* `width`, `height`, and `sizes` — will be passed to the template as HTML attributes. | |
* | |
* Toggle activates `@Input() elementType: 'img' | 'picture' = 'picture';` in `MediaComponent` | |
* | |
* Toggle sets `elemetType` to `img` for all `MediaComponent` usages, where one of the following format is set: `cartIcon`, `thumbnail`, `product`, `zoom` | |
*/ |
...frontlib/cms-components/product/carousel/product-carousel/product-carousel.component.spec.ts
Outdated
Show resolved
Hide resolved
projects/storefrontlib/cms-components/product/product-images/product-images.component.html
Outdated
Show resolved
Hide resolved
pawelfras
reviewed
Oct 17, 2024
pawelfras
approved these changes
Oct 17, 2024
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.
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: https://jira.tools.sap/browse/CXSPA-7700
QA steps for new application
QA steps for migrated app:
It's worth to perform similar QA steps for migrated app. The following is how to prepare migrated app:
checkout 6.8.0 in Spartacus repo
build and deploy Spartacus packages locally:
npx ts-node ./tools/schematics/testing.ts in the SPA root folder`
create new app npx @angular/cli@15 new my-app --style=scss --routing=false
go to the project's directory and install Spartacus from local packages:
npx @angular/cli@15 add @spartacus/schematics@latest --baseUrl="https://40.76.109.9:9002"
checkout feat/CXSPA-7700 in Spartacus repom build and deploy packages with changes
follow steps from 2211-migration.md to migrate app
perform QA
The idea behind this PR is to provide flexibility for customers, so they can decide whether they want to use the
Crucial parts:
@Input() elementType: 'img' | 'picture' = 'img';
@Input() width: number;
@Input() height: number;
@Input() sizes: string;
pictureElementFormats
in media configpictureFormatsOrder
in media configgetMediaForPictureElement()
in MediaServiceresolveSources()
in MediaServiceuseExtendedMediaComponentConfiguration?: boolean;
feature toggleuseLegacyMediaComponent
config property andUSE_LEGACY_MEDIA_COMPONENT
injection token### Important:
After activation
useExtendedMediaComponentConfiguration
toggle default HTML element in MediaComponent will be<img>
Only BannerComponent has passed
'picture'
value. If you need to use<picture>
HTML elementyou need to pass
[elementType]="'picture'"
to<cx-media>
The idea is to move away from the concept that
<img>
is legacy, because from my research, I understand that both elements make sense in different situations. The<picture>
element is the only way to go in situations when we what to handle different art directions or more complex logic, more complex media queries. That's why I want to provide flexibility, so users can decide where they need<picture>
element.In our application I think we could enable it for banner component, since it has different formats, and I see a possibility here to add in future additional formats to handle retina screens and to improve the visibility of this banners on retina mobile/desktop devices.
Current default-media.config is more for representation of the approach, and should be reduced to only formats that we have and queries that we use before merge.