aemsites / stericycle-shared

Edge Delivery Service Site for Stericycle sites
https://www.shredit.com, https://www.stericycle.com
Apache License 2.0
0 stars 1 forks source link

Narrow icon changes #502

Closed jindaliiita closed 1 week ago

jindaliiita commented 1 week ago

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

Fix #466

Test URLs:

aem-code-sync[bot] commented 1 week ago

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed. In case there are problems, just click a checkbox below to rerun the respective action.

Commits * [ea51b11](https://github.com/aemsites/stericycle-shared/commit/ea51b11e59da6cc213ac69f7f1b67e4ace335c86) :white_check_mark: (latest) * [e8899c1](https://github.com/aemsites/stericycle-shared/commit/e8899c107eb72aeaea94cc1fa43397f6564da13c) :white_check_mark: * [4800679](https://github.com/aemsites/stericycle-shared/commit/4800679d3cb9bbd13a8a8c83a5baa0b8fb49c9e2) :white_check_mark: * [9837768](https://github.com/aemsites/stericycle-shared/commit/9837768707e28c9688acf98b0811ee4a15152e6d) :white_check_mark:
aem-code-sync[bot] commented 1 week ago
Page Scores Audits Google
:iphone: /en-us/secure-shredding-services/one-off-shredding-service PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
:desktop_computer: /en-us/secure-shredding-services/one-off-shredding-service PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
jindaliiita commented 1 week ago

This needs a separate smaller icon size for tablet / mobile. Also you should probably use var(--icon-size-xl) instead of the hardcoded 4rem. Otherwise LGTM

For small devices, they seem to be too small, which is why I maintained similar behavior for all screens.

kronnox commented 1 week ago

For small devices, they seem to be too small, which is why I maintained similar behavior for all screens.

You are right. I adjusted my PR to match this behaviour.

The icon sizing for the column blocks seems to be very inconsistently done in general. Sometime it's set on the wrapper, sometimes on the image. This is bad. I think we wouldn't even need so many variants of the column block if we would just be using the xl-icons style on the blocks / sections instead. But I don't really want to touch this, since it feels like it could break a lot of stuff.

bstopp commented 1 week ago

@kronnox - is your review change required still valid? if not can you dismiss it?