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.

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

fix(styles): restore use of globals/imports.scss #12175

Merged
merged 3 commits into from
Dec 23, 2024
Merged

Conversation

andy-blum
Copy link
Member

Description

the carbon reset has all items box-sizing set to inherit. this changes that to border box and is intended mainly to see what shifts in Percy

@andy-blum andy-blum requested a review from a team as a code owner December 17, 2024 18:58
@andy-blum andy-blum requested review from m4olivei, bruno-amorim, marcelojcs and Valentin-Sorin-Nicolae and removed request for a team December 17, 2024 18:58
Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for ibm-dotcom-web-components-react-wrap ready!

Name Link
🔨 Latest commit 1a89143
🔍 Latest deploy log https://app.netlify.com/sites/ibm-dotcom-web-components-react-wrap/deploys/67630c6e31aacc0008c43f68
😎 Deploy Preview https://deploy-preview-12175--ibm-dotcom-web-components-react-wrap.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for ibm-dotcom-web-components ready!

Name Link
🔨 Latest commit 1a89143
🔍 Latest deploy log https://app.netlify.com/sites/ibm-dotcom-web-components/deploys/67630c6e5b86750008623e4a
😎 Deploy Preview https://deploy-preview-12175--ibm-dotcom-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@andy-blum
Copy link
Member Author

This did nothing.

@andy-blum andy-blum closed this Dec 17, 2024
@andy-blum
Copy link
Member Author

The components most impacted in the percy review:

Locale Modal

Component fails grid alignment in both cases.

Before:
Screenshot 2024-12-18 at 8 44 30 AM

After:
Screenshot 2024-12-18 at 8 44 44 AM

Expressive Modal

Component fails grid alignment in both cases.

Before:
Screenshot 2024-12-18 at 8 46 03 AM

After:
Screenshot 2024-12-18 at 8 46 08 AM

Card Section Simple

Component fails grid alignment in both cases.

Before:
Screenshot 2024-12-18 at 8 54 32 AM

After:
Screenshot 2024-12-18 at 8 54 43 AM

Quote

Component is improved by this PR

Before:
Screenshot 2024-12-18 at 8 57 06 AM

After:
Screenshot 2024-12-18 at 8 57 17 AM

Link List Section

Component is improved by this PR

Before:
Screenshot 2024-12-18 at 8 58 46 AM

After:
Screenshot 2024-12-18 at 8 58 50 AM

@andy-blum andy-blum changed the title fix(everything): box-sizing border-box fix(styles): restore use of globals/imports.scss Dec 18, 2024
@areagan1030
Copy link

  • Locale modal – it appears that the internal spacing is correct, but the outer container is shifting in how it sits on the grid on md to max (sm is as expected). I don't see v2 Figma specs, but I'm looking at v1 and the modal is not sitting on the grid in a normal way here either. So, storybook preview does actually align with the v1 Figma layout.
  • Expressive modal – same as above. The v1 Figma is looking closer to insetting the modal 4 cols on lg to max, and md is somewhat off grid. While the storybook preview is also off grid, these are both a little wonky in different ways.
  • Card Section Simple – the leftmost alignment of the cards is looking okay (I'm assuming borders might be set to outside, which makes this look shifted 1px left – but text alignment is correct). It appears that the card width and the spacing between cards is off. This is dependent on whether the section should be using the default or narrow grid mode. If default, card edges align on the grid left and right (4 cols) with 2rem between. If condensed, left edge hangs 1rem in gutter and right edge is on grid with 1rem between cards. I assume default may be the preferred for CIBM here, but worth noting that we're not using default grid in AEM (generally has been condensed or narrow).
  • Quote – For md to max, the text should be aligning on the grid line with the quote hanging to the gutter at left. Design specs show quote hanging 1 rem to left. It also appears that the rule at the bottom of the component is hanging 1rem into gutter left and right. So this aligns with the quote mark at left. And the rule in storybook preview looks like it's stopping short on the right side.
  • Link list section – This one seems to be matching the design specs. In AEM, we had been following the narrow grid standards previously with the horizontal rule, but it appears this doesn't hang in the gutter in v2 now. The storybook preview seems to have the correct grid alignment.

@areagan1030
Copy link

Chatted with @andy-blum — we are good with everything here today, with the caveat that Card Section will get a little more tweaking for its grid.

@andy-blum andy-blum requested a review from m4olivei December 23, 2024 14:48
@m4olivei m4olivei added the Ready to merge Label for the pull requests that are ready to merge label Dec 23, 2024
@kodiakhq kodiakhq bot merged commit 30de2fd into main Dec 23, 2024
19 checks passed
@kodiakhq kodiakhq bot deleted the fix/box-sizing branch December 23, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants