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

feat: Configurable media component #19067

Merged
merged 62 commits into from
Oct 18, 2024
Merged

feat: Configurable media component #19067

merged 62 commits into from
Oct 18, 2024

Conversation

rmch91
Copy link
Contributor

@rmch91 rmch91 commented Jul 23, 2024

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 element or the element, and to provide a separate config for the element that will allow them to add specific media queries.

Crucial parts:

  1. New input in media component, @Input() elementType: 'img' | 'picture' = 'img';
  2. New input in media component, width @Input() width: number;
  3. New input in media component, height @Input() height: number;
  4. New input in media component, sizes @Input() sizes: string;
  5. pictureElementFormats in media config
  6. pictureFormatsOrder in media config
  7. getMediaForPictureElement() in MediaService
  8. resolveSources() in MediaService
  9. useExtendedMediaComponentConfiguration?: boolean; feature toggle
  10. deprecation of useLegacyMediaComponent config property and USE_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 element
you 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.

@rmch91 rmch91 requested a review from a team as a code owner July 23, 2024 12:27
@github-actions github-actions bot marked this pull request as draft July 23, 2024 12:27
@rmch91 rmch91 marked this pull request as ready for review July 31, 2024 11:15
@rmch91 rmch91 requested a review from a team as a code owner July 31, 2024 11:15
Copy link

cypress bot commented Jul 31, 2024

spartacus    Run #45383

Run Properties:  status check passed Passed #45383  •  git commit 09eaa5db29 ℹ️: Merge 1af972d8dd49e08e59d5832b7d3582eaa5158053 into 6d20b3be1923cc0b44332e18c3c3...
Project spartacus
Run status status check passed Passed #45383
Run duration 03m 55s
Commit git commit 09eaa5db29 ℹ️: Merge 1af972d8dd49e08e59d5832b7d3582eaa5158053 into 6d20b3be1923cc0b44332e18c3c3...
Committer Roman
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 125

@rmch91 rmch91 changed the title Feat/cxspa 7700 feat: Configurable media component Aug 1, 2024
@github-actions github-actions bot marked this pull request as draft August 1, 2024 09:36
@rmch91 rmch91 marked this pull request as ready for review October 17, 2024 10:05
@rmch91 rmch91 requested review from a team as code owners October 17, 2024 10:05
@github-actions github-actions bot marked this pull request as draft October 17, 2024 11:03
@rmch91 rmch91 marked this pull request as ready for review October 17, 2024 12:05
@github-actions github-actions bot marked this pull request as draft October 17, 2024 12:22
@rmch91 rmch91 marked this pull request as ready for review October 17, 2024 12:22
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`
*/
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
/**
* 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`
*/

This reverts commit 28d31c9.
This reverts commit bdbd15e.
This reverts commit 0641aaf.
This reverts commit 7c7068b.
@github-actions github-actions bot marked this pull request as draft October 17, 2024 13:21
@rmch91 rmch91 marked this pull request as ready for review October 17, 2024 13:51
Copy link
Contributor

@pawelfras pawelfras left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot marked this pull request as draft October 17, 2024 18:34
@rmch91 rmch91 marked this pull request as ready for review October 17, 2024 19:44
@rmch91 rmch91 merged commit 1499ec5 into develop Oct 18, 2024
28 checks passed
@rmch91 rmch91 deleted the feat/CXSPA-7700 branch October 18, 2024 07:52
@Platonn Platonn restored the feat/CXSPA-7700 branch October 21, 2024 10:20
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.

2 participants