Pencil Promo block #108 #195
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
markovukiceviccn
requested review from
Lakshmishri,
SantiagoHomps-NC,
TomaszDziezykNetcentric,
cogniSyb and
taimurCognizant
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
Lakshmishri
requested changes
Dec 28, 2023
There was a problem hiding this comment.
Min height for this should be removed , it should just have padding like here
Should the component be full width, so if screen is greater than 1440px, there background for entire block ? @cogniSyb , @DanielaPedrochevd ?
For screen size 1200px - 1400px, margin-left: 105px required, so it's aligned with rest of the content in page
|
IDK if this is regarding this issue but since the block is on this page I found it and could be solved. There is a small gap in the images of the 'all-trucks' block when hovered. This could be solved by deleting line 288 ( |
|
cogniSyb
requested changes
Jan 3, 2024
| background: var(--c-primary-gold); | ||
| } | ||
|
|
||
| .v2-pencil-promo__promo-banner .icon-arrow-right svg { |
There was a problem hiding this comment.
Use .icon instead of .icon-arrow-right
| } | ||
|
|
||
| .v2-pencil-promo__pencil-banner .v2-pencil-promo__content { | ||
| width: 45%; |
There was a problem hiding this comment.
Should be 512px, which is 1040/2 - 16px gutter
| /* Pencil banner styles */ | ||
| .v2-pencil-promo__pencil-banner .v2-pencil-promo__content { | ||
| background: var(--c-primary-black); | ||
| min-height: 147px; |
There was a problem hiding this comment.
Remove all the min-height
|
|
||
| .v2-pencil-promo__pencil-banner .v2-pencil-promo__content p { | ||
| display: inline; | ||
| font: 1.5625rem/117% var(--ff-body); |
There was a problem hiding this comment.
Should be --ff-subheadings-medium instead
| @media (min-width: 1200px) { | ||
| .v2-pencil-promo__pencil-banner { | ||
| max-width: 1440px; | ||
| margin: 20px auto; |
There was a problem hiding this comment.
We shouldn’t have margin here
| [data-pencil-banner-black = 'disabled'] .v2-pencil-promo--pencil-banner-black, | ||
| [data-pencil-banner-copper = 'disabled'] .v2-pencil-promo--pencil-banner-copper, | ||
| [data-promo-block-gold = 'disabled'] .v2-pencil-promo--promo-banner-gold, | ||
| [data-promo-block-copper = 'disabled'] .v2-pencil-promo--pencil-banner-copper { |
There was a problem hiding this comment.
I don’t see the use of the data attributes right now. I think that the following acceptance criterium caused a bit of confusion:
As an editor I can select between the four variants (black and copper pencil banners or the promo block copper or gold).
| text-decoration-color: var(--c-primary-white); | ||
| } | ||
|
|
||
| .v2-pencil-promo--pencil-banner-copper .v2-pencil-promo__content a:hover { |
There was a problem hiding this comment.
The hover state should be activated when hovering the entire block, as it is entirely clickable.
| min-height: 76px; | ||
| padding: 12px 16px; | ||
| margin: 0 76px 0 0; | ||
| font: var(--headline-5-font-size)/var(--headline-5-line-height) var(--ff-body); |
There was a problem hiding this comment.
Should be --ff-subheadings-medium instead. The font-size should be bigger on desktop
|
|
||
| /* Generic block styles */ | ||
| .v2-pencil-promo { | ||
| margin: 20px 0; |
There was a problem hiding this comment.
We shouldn’t have margin here, only for the variant with an image
jonatan-lledo-netcentric
assigned Lakshmishri and cogniSyb and unassigned markovukiceviccn, Lakshmishri and cogniSyb
cogniSyb
requested changes
Jan 8, 2024
|
|
||
| .v2-pencil-promo__pencil-banner .v2-pencil-promo__content { | ||
| width: 512px; | ||
| margin-left: 200px; |
| background: var(--c-accent-copper); | ||
| } | ||
|
|
||
| .v2-pencil-promo--pencil-banner-black .v2-pencil-promo__content-wrapper { |
There was a problem hiding this comment.
Remove this declaration block
| } | ||
|
|
||
| @media (min-width: 1200px) { | ||
| .v2-pencil-promo__pencil-banner .v2-pencil-promo__content-wrapper { |
There was a problem hiding this comment.
Remove this declaration block
| } | ||
|
|
||
| /* Pencil banner styles */ | ||
| .v2-pencil-promo__pencil-banner .v2-pencil-promo__content { |
There was a problem hiding this comment.
Remove .v2-pencil-promo__content from selector and put padding: 30px 16px; in its own declaration block with .v2-pencil-promo__pencil-banner .v2-pencil-promo__content as selector.
| /* Pencil banner styles */ | ||
| .v2-pencil-promo__pencil-banner .v2-pencil-promo__content { | ||
| background: var(--c-primary-black); | ||
| color: var(--c-primary-white); |
There was a problem hiding this comment.
Utilise --text-color here
|
|
||
| /* stylelint-disable-next-line no-descending-specificity */ | ||
| .v2-pencil-promo--pencil-banner-copper .v2-pencil-promo__content a { | ||
| text-decoration-color: var(--c-primary-white); |
| } | ||
|
|
||
| .v2-pencil-promo__promo-banner .icon svg { | ||
| --color-icon: var(--c-primary-white); |
| height: 24px; | ||
| } | ||
|
|
||
| .v2-pencil-promo--promo-banner-copper .icon-arrow-right { |
There was a problem hiding this comment.
Change .icon-arrow-right into .icon
| } | ||
|
|
||
| .v2-pencil-promo__promo-banner .v2-pencil-promo__content, | ||
| .v2-pencil-promo__promo-banner .v2-pencil-promo__content p { |
There was a problem hiding this comment.
Update this selector as you’re moving the background-color to a different selector
cogniSyb
approved these changes
Jan 9, 2024
jonatan-lledo-netcentric
assigned SantiagoHomps-NC and unassigned Lakshmishri and cogniSyb
SantiagoHomps-NC
approved these changes
Jan 10, 2024
jonatan-lledo-netcentric
approved these changes
Jan 11, 2024
Lakshmishri
approved these changes
Jan 11, 2024
manuel-vara
added a commit
that referenced
this pull request
Jan 31, 2024
* Update fstab.yaml for redesign * change url and sharepoint to upstream urls * Inpage navigation is in reversed order on Firefox #173 (#178) * fix Firefox issue with sorting * fix naming issues * refactor focus state, button styles * fix overlap issue * Hero V2 Block displays different images for mobile and desktop. #106 (#179) * refactor naming structure * refactor adaptive images mechanism * Icon cards block (#181) * #509 Fix displaying header on dealer page * fix done * one trust position ix * Embed block match NZ one * Update feed.xml * Fix createElement() v1-Cards * Update feed.xml * Update release.yml * columns block adapted * icon cards working * css clean * adjustements * last changes * specific class * btn state styling * comments corrected * secondary btn margin corrected * secondary btn class added * cleanup --------- Co-authored-by: Tomasz Dziezyk <[email protected]> Co-authored-by: TomaszDziezykNetcentric <[email protected]> Co-authored-by: Jonatan Lledo <[email protected]> Co-authored-by: aem-code-sync[bot] <aem-code-sync[bot]@users.noreply.github.com> Co-authored-by: Syb <[email protected]> * The pop-up of the video displayed clicking on the Play video button is smaller and the thumbnail is cutout #182 (#183) * iPhone/iPad - Icon Cards - The local video in the video pop-up cannot be played and is incorrectly displayed #184 (#185) * fix #184: use <video> tag for video in modal * fix modal close button * fix override fullscreen play for safari * refactor setting attributes * Opening up Images will display an incorrect default state on the Image grid carousel #169 (#193) * Hero variant for Solutions page #45 (#197) * hero variants added * refactored changes to alltrucks block * comments addressed * color confusion * make btn color always white * button style refactor * margin solved * pdp edge cases * Tabbed Carousel variant for wheelbase #190 (#196) * add new variant for tabbed carousel * 190 fadeIn effect with no scroll * 190 Fix line more than 100 chars * 190 fix index overflow * Figcap conditional add, default color & tab items wider * list style none & setCarousel index-1 too --------- Co-authored-by: Jonatan Lledo <[email protected]> * Pencil Promo block #108 (#195) * #108 - Pencil promo and banner promo block * #108 - Add cursor: pointer to the block * #108 - Add pencil banner links styles * #108 - Change link hover color to acheive better contrast * Applied comments & each block cares by itself only * fixed alignment issue in line 288 * fix lint issue * Fix an issue if p Element is not present inside content * Style issues fix * remove an unnecessary search for h1 & its styling * move the hover effect 1 level up & a bit of code clean up --------- Co-authored-by: Jonatan Lledo <[email protected]> * fix alignment, add hover and focus state * V2 Columns Block variation #133 (#186) --------- Co-authored-by: aem-code-sync[bot] <aem-code-sync[bot]@users.noreply.github.com> Co-authored-by: Lakshmishri <[email protected]> Co-authored-by: Syb Wartna <[email protected]> * Add "My Assets" functionality #201 (#202) * add my assets to sidekick * add custom viewport configurations for sidekick library * Powertrain V2 Slider Block #147 (#200) * add block * add new section background color --------- Co-authored-by: Syb <[email protected]> Co-authored-by: Syb Wartna <[email protected]> Co-authored-by: SantiagoHomps-NC <[email protected]> Co-authored-by: Tomasz Dziezyk <[email protected]> Co-authored-by: TomaszDziezykNetcentric <[email protected]> Co-authored-by: Jonatan Lledo <[email protected]> Co-authored-by: aem-code-sync[bot] <aem-code-sync[bot]@users.noreply.github.com> Co-authored-by: taimurCognizant <[email protected]> Co-authored-by: Lakshmishri <[email protected]> Co-authored-by: Marko Vukićević <[email protected]>
cogniSyb
added a commit
that referenced
this pull request
Feb 26, 2024
* change url and sharepoint to upstream urls * Release 4 (#211) * Update fstab.yaml for redesign * change url and sharepoint to upstream urls * Inpage navigation is in reversed order on Firefox #173 (#178) * fix Firefox issue with sorting * fix naming issues * refactor focus state, button styles * fix overlap issue * Hero V2 Block displays different images for mobile and desktop. #106 (#179) * refactor naming structure * refactor adaptive images mechanism * Icon cards block (#181) * #509 Fix displaying header on dealer page * fix done * one trust position ix * Embed block match NZ one * Update feed.xml * Fix createElement() v1-Cards * Update feed.xml * Update release.yml * columns block adapted * icon cards working * css clean * adjustements * last changes * specific class * btn state styling * comments corrected * secondary btn margin corrected * secondary btn class added * cleanup --------- Co-authored-by: Tomasz Dziezyk <[email protected]> Co-authored-by: TomaszDziezykNetcentric <[email protected]> Co-authored-by: Jonatan Lledo <[email protected]> Co-authored-by: aem-code-sync[bot] <aem-code-sync[bot]@users.noreply.github.com> Co-authored-by: Syb <[email protected]> * The pop-up of the video displayed clicking on the Play video button is smaller and the thumbnail is cutout #182 (#183) * iPhone/iPad - Icon Cards - The local video in the video pop-up cannot be played and is incorrectly displayed #184 (#185) * fix #184: use <video> tag for video in modal * fix modal close button * fix override fullscreen play for safari * refactor setting attributes * Opening up Images will display an incorrect default state on the Image grid carousel #169 (#193) * Hero variant for Solutions page #45 (#197) * hero variants added * refactored changes to alltrucks block * comments addressed * color confusion * make btn color always white * button style refactor * margin solved * pdp edge cases * Tabbed Carousel variant for wheelbase #190 (#196) * add new variant for tabbed carousel * 190 fadeIn effect with no scroll * 190 Fix line more than 100 chars * 190 fix index overflow * Figcap conditional add, default color & tab items wider * list style none & setCarousel index-1 too --------- Co-authored-by: Jonatan Lledo <[email protected]> * Pencil Promo block #108 (#195) * #108 - Pencil promo and banner promo block * #108 - Add cursor: pointer to the block * #108 - Add pencil banner links styles * #108 - Change link hover color to acheive better contrast * Applied comments & each block cares by itself only * fixed alignment issue in line 288 * fix lint issue * Fix an issue if p Element is not present inside content * Style issues fix * remove an unnecessary search for h1 & its styling * move the hover effect 1 level up & a bit of code clean up --------- Co-authored-by: Jonatan Lledo <[email protected]> * fix alignment, add hover and focus state * V2 Columns Block variation #133 (#186) --------- Co-authored-by: aem-code-sync[bot] <aem-code-sync[bot]@users.noreply.github.com> Co-authored-by: Lakshmishri <[email protected]> Co-authored-by: Syb Wartna <[email protected]> * Add "My Assets" functionality #201 (#202) * add my assets to sidekick * add custom viewport configurations for sidekick library * Powertrain V2 Slider Block #147 (#200) * add block * add new section background color --------- Co-authored-by: Syb <[email protected]> Co-authored-by: Syb Wartna <[email protected]> Co-authored-by: SantiagoHomps-NC <[email protected]> Co-authored-by: Tomasz Dziezyk <[email protected]> Co-authored-by: TomaszDziezykNetcentric <[email protected]> Co-authored-by: Jonatan Lledo <[email protected]> Co-authored-by: aem-code-sync[bot] <aem-code-sync[bot]@users.noreply.github.com> Co-authored-by: taimurCognizant <[email protected]> Co-authored-by: Lakshmishri <[email protected]> Co-authored-by: Marko Vukićević <[email protected]> * change url and sharepoint to upstream urls --------- Co-authored-by: Syb Wartna <[email protected]> Co-authored-by: Lakshmishri <[email protected]> Co-authored-by: Syb <[email protected]> Co-authored-by: SantiagoHomps-NC <[email protected]> Co-authored-by: Tomasz Dziezyk <[email protected]> Co-authored-by: TomaszDziezykNetcentric <[email protected]> Co-authored-by: Jonatan Lledo <[email protected]> Co-authored-by: aem-code-sync[bot] <aem-code-sync[bot]@users.noreply.github.com> Co-authored-by: taimurCognizant <[email protected]> Co-authored-by: Marko Vukićević <[email protected]>
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.
Fix #108
Banner blocks can be enabled or disabled by changing the data attributes in page word file:Section MetadataPencil banner black | disabledPencil banner copper | pencil-banner-copperPromo block gold | promo-banner-goldPromo block copper | promo-banner-coppereach block now is independent and Section metadata is no longer needed, but author is responsible to add the proper variant to the block
Also Fix
in All-trucks block had a tiny style issue, that now is fixed
Test URLs: