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.

Sign up for GitHub

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

Jump to bottom

feature(stats-bar): Added statsbar block #320

Merged
merged 12 commits into from
Sep 7, 2023
Merged

feature(stats-bar): Added statsbar block #320

merged 12 commits into from
Sep 7, 2023

Conversation

sahmad-merative
Copy link

@sahmad-merative sahmad-merative commented Aug 23, 2023 •
edited by proeung
Loading

Design Specs

If applicable, add the direct link to the design specs of the component/feature that's part of this PR.

Test URLs

Testing Instruction

image

If applicable, please describe the tests that you ran to verify your changes. Provide instructions and link to the hlx deploy preview so that QA and the design team can provide proper testing.

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 23, 2023

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.

  • Re-run PSI Checks

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 23, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 23, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 23, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

sharathmrft
sharathmrft suggested changes Aug 24, 2023
View reviewed changes
Copy link

@sharathmrft sharathmrft left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@sahmad-merative @proeung
Few observations on Stats bar as below

1.Button color should be white but it is showing in Blue color
2.Logos are breaking in mobile and tab view for more than 4 Logos refer screenshot below
image (23)
image (22)

image

@proeung proeung added Needs design QA PRs on feature requests and new components have to get design approval before merge. 🛠️ fix needed labels Aug 24, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Aug 28, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

proeung
proeung suggested changes Aug 28, 2023
View reviewed changes
Copy link

@proeung proeung left a comment

Choose a reason for hiding this comment

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

@sahmad-merative See my comments below .

Screenshot 2023-08-28 at 4 32 14 PM

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 29, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 29, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 30, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 30, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 30, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

proeung
proeung suggested changes Aug 30, 2023
View reviewed changes
Copy link

@proeung proeung left a comment

Choose a reason for hiding this comment

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

Added some code suggestions as well as questions about some the approaches.

@sachinmesh Can you help conduct a full design QA for this PR? Thanks!

blocks/stat/stat.css Outdated Show resolved Hide resolved
blocks/carousel/carousel.css Show resolved Hide resolved
blocks/carousel/carousel.css Show resolved Hide resolved
blocks/carousel/carousel.css Show resolved Hide resolved
blocks/carousel/carousel.css Outdated Show resolved Hide resolved
blocks/stat/stat.css Outdated Show resolved Hide resolved
blocks/stat/stat.css Outdated Show resolved Hide resolved
blocks/stat/stat.js Outdated Show resolved Hide resolved
@aem-code-sync
Copy link

aem-code-sync bot commented Aug 31, 2023

Page Scores Audits Google
/block-library/templates/statsbar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

proeung
proeung approved these changes Aug 31, 2023
View reviewed changes
Copy link

@proeung proeung left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@sahmad-merative Looks good from a code perspective and thanks for taking my code suggestions.

I'd like to get @sachinmesh's full design QA on this PR before we mark this as "Ready to Merge. @sachinmesh Can you take a look at this PR when you have a moment, thanks! Also, @sharathmrft can you take a look as well?

https://783-stats-bar--merative2--sahmad-merative.hlx.page/block-library/sections/section-stats-bar

@sharathmrft
Copy link

sharathmrft commented Sep 1, 2023 •
edited
Loading

@sahmad-merative No divider in between each block at the top horizontal line screen shot attached here.
image

@proeung

@sachinmesh
Copy link

  1. On desktop, this should be Alliance no.116px regular and not light.
  2. Spacing below the inline link CTA and divider line should be 64px and not 48px.
image
  1. The stat block should be of 216px wide
image
  1. Againon tablet and mobile the font style should be regular and not light.
image

CC: @proeung @sahmad-merative @sharathmrft

@proeung proeung added 🛠️ fix needed and removed Needs design QA PRs on feature requests and new components have to get design approval before merge. labels Sep 1, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Sep 4, 2023

Page Scores Audits Google
/block-library/sections/section-stats-bar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

sachinmesh
sachinmesh approved these changes Sep 4, 2023
View reviewed changes
Copy link

@sachinmesh sachinmesh left a comment

Choose a reason for hiding this comment

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

Good to go.
@proeung @sahmad-merative @sharathmrft

sharathmrft
sharathmrft approved these changes Sep 5, 2023
View reviewed changes
Copy link

@sharathmrft sharathmrft left a comment

Choose a reason for hiding this comment

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

@sahmad-merative Looks good

@proeung
Copy link

proeung commented Sep 6, 2023

@sahmad-merative Looks like there are some merge conflicts coming from main. Once, these conflicts have been resolved, I'll approve and merge your PR.

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 7, 2023

Page Scores Audits Google
/block-library/sections/section-stats-bar PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@proeung proeung added Ready to merge Label for the pull requests that are ready to merge and removed 🛠️ fix needed labels Sep 7, 2023
@proeung proeung merged commit 64448a1 into hlxsites:main Sep 7, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharathmrft sharathmrft sharathmrft approved these changes

@sachinmesh sachinmesh sachinmesh approved these changes

@proeung proeung proeung approved these changes

@nimithshetty17 nimithshetty17 Awaiting requested review from nimithshetty17

Assignees
No one assigned
Labels
Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sahmad-merative @sharathmrft @sachinmesh @proeung