hlxsites / merative2

Merative.com site on Franklin
https://merative.com
Apache License 2.0
2 stars 9 forks source link

fix(design-qa): Fix spacing issues within Content Intro & minor bugs seen within Microsite #341

Closed proeung closed 9 months ago

proeung commented 9 months ago

Issue

Fixes - https://jira.sdlc.merative.com/browse/MERATIVE-887

Description

Changed

Design Specs

Test URLs for spacing issues

Test URLs for FE bugs seen in Microsite Missouri Landing

Testing Instruction

aem-code-sync[bot] commented 9 months ago

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed. In case there are problems, just click the checkbox below to rerun the respective action.

aem-code-sync[bot] commented 9 months ago
Page Scores Audits Google
/campaigns/curam/missouri PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/curam PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
keith-kaplan commented 9 months ago

@proeung this is looking great. A question for content entry on this page. https://fix-space-qa-887--merative2--proeung.hlx.page/curam (https://main--merative2--hlxsites.hlx.page/curam)

Should the content checkmarks section metadata still be there as it's own section? it looks like the spacing is working fine, but wanted to ensure the content entry didn't need to be one section.

image
proeung commented 9 months ago

Should the content checkmarks section metadata still be there as it's own section? it looks like the spacing is working fine, but wanted to ensure the content entry didn't need to be one section.

@keith-kaplan It should be in the same section. I forgot to move the stat into the same section as the Content Intro that has the checkmarks. Feel free to change this.

proeung commented 9 months ago

@sachinmesh Which section are you referring to for "We can reduce this padding to 96px instead of 160px."? The screenshot that you've provided doesn't look correct.

This component is breaking

This is an issue that's not scoped to this PR and will be handled in a separate ticket.

aem-code-sync[bot] commented 9 months ago
Page Scores Audits Google
/campaigns/curam/missouri PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/curam PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
sachinmesh commented 9 months ago

This padding @proeung between the title and the first simple component.

image
aem-code-sync[bot] commented 9 months ago
Page Scores Audits Google
/campaigns/curam/missouri PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/curam PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
proeung commented 9 months ago

@sachinmesh I just pushed a commit that should address your feedback about the spacing. Can you take a look again?

sachinmesh commented 9 months ago

This is good to go. Just one thing if we can update the title here to H1 instead of H2, as there is no H1 title on the page. @proeung @keith-kaplan

image
proeung commented 9 months ago

@sachinmesh Thanks for the review! Regarding your question, I think we should keep with the <h2> tag and styling from the DS instead of trying to mimic the style/specs of <h1> for this particular case. This will keep the heading size consistent per section and not create a one-off case in our stylesheet to keep with the page DOM structure and SEO. @keith-kaplan Let me know if you agree.

Screenshot 2023-09-25 at 1 17 08 PM