Mwpw 157587 | [Geo-Routing][Page Performance] Removing the georouting modal background image #2891
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
aishwaryamathuria
requested review from
narcis-radu,
Blainegunn,
meganthecoder,
mokimo and
robert-bogos
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2891 +/- ##
=======================================
Coverage 96.23% 96.24%
=======================================
Files 237 236 -1
Lines 54242 54200 -42
=======================================
- Hits 52198 52163 -35
+ Misses 2044 2037 -7 ☔ View full report in Codecov by Sentry. |
aishwaryamathuria
requested review from
chrischrischris,
bandana147,
vgoodric,
salonijain3 and
jedjedjedM
aishwaryamathuria
added
needs-verification
|
@aishwaryamathuria do we still need any of https://github.com/adobecom/milo/tree/stage/libs/features/georoutingv2/img ? The background images sound like they can be removed as well |
| @@ -140,19 +136,6 @@ | |||
| padding: initial; | |||
| } | |||
|
|
|||
| @media (min-width: 480px) { | |||
| .dialog-modal.locale-modal-v2 { | |||
| background-image: url('/libs/features/georoutingv2/img/GeoModal_BG_Map_Tablet.png'); | |||
There was a problem hiding this comment.
we need to remove the image from assets
There was a problem hiding this comment.
@narcis-radu I have removed this image from repo
|
|
||
| @media (min-width: 1200px) { | ||
| .dialog-modal.locale-modal-v2 { | ||
| background-image: url('/libs/features/georoutingv2/img/GeoModal_BG_Map_Desktop.png'); |
There was a problem hiding this comment.
we need to remove the image from assets
There was a problem hiding this comment.
@narcis-radu I have removed this image from repo
|
@aishwaryamathuria - let's remove the images if they aren't used anymore. Thank you! |
|
@narcis-radu @robert-bogos @mokimo |
| svgDiv = createTag('div', { class: 'georouting-bg' }, getMobileBg()); | ||
| } | ||
| const georoutingWrapper = createTag('div', { class: 'georouting-wrapper fragment', style: 'display:none;' }, svgDiv); | ||
| const georoutingWrapper = createTag('div', { class: 'georouting-wrapper fragment', style: 'display:none;' }); |
There was a problem hiding this comment.
Unrelated to this PR, but I see the display declaration is being kept and overridden by CSS with !important. Does this need to stay like this? Could we use a modifier class instead of adding the style via JS? Could we remove the style attribute at a later point? It's just a bit confusing to see
There was a problem hiding this comment.
@overmyheadandbody This would currently be required as the getDetails function that fetches and adds the modal to the DOM is called before showModal that fetches the georoutingv2.css and till that time the modal needs to be hidden. We would either need to add this as class to a different css file that gets loaded before and then apply this class here.
We can probably explore this as a part of a different PR? Could you share your thoughts on the same?
@narcis-radu @mokimo @robert-bogos Could you share your inputs on the same?
|
This PR is currently in the |
|
Validation done on the below url After the fix Validation in locale page page speed before page speed after CC : @sigadamvenkata , @salonijain3 |
spadmasa
added
verified
In Current Milo pages with low contrast background image in LCP (https://www.adobe.com/products/photoshop.html) or having smaller subject image in LCP the georouting modal is getting tagged as LCP.
Browser keeps looking for LCP till user interacts with the page and hence once Geo Modal comes up in such cases the modal gets tagged increasing the LCP count and impacting the page performance.
As per the latest design decision update in the ticket https://jira.corp.adobe.com/browse/MWPW-157587 opening this PR to remove the georouting image from the modal for all Milo pages.
Resolves: MWPW-157587
Test URLs: