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

chore(content-group-pictogram): syncing stories and spacing updates #7314

Merged
merged 12 commits into from
Nov 18, 2021
Merged

chore(content-group-pictogram): syncing stories and spacing updates #7314

merged 12 commits into from
Nov 18, 2021

Conversation

guilhermelMoraes
Copy link
Contributor

Related Ticket(s)

[Content group pictograms] Storybooks QA #5666

@guilhermelMoraes guilhermelMoraes requested a review from a team as a code owner October 5, 2021 15:20
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 5, 2021 •
edited
Loading

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7314/index.html

Built with commit: 84318d62c05f30c45bb0b18f1e76b3c06efe9fbd

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 5, 2021 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7314/index.html

Built with commit: 84318d62c05f30c45bb0b18f1e76b3c06efe9fbd

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 5, 2021 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/7314/index.html

Built with commit: 84318d62c05f30c45bb0b18f1e76b3c06efe9fbd

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 5, 2021 •
edited
Loading

Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7314/index.html

Built with commit: 84318d62c05f30c45bb0b18f1e76b3c06efe9fbd

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 5, 2021 •
edited
Loading

Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7314/index.html

Built with commit: 84318d62c05f30c45bb0b18f1e76b3c06efe9fbd

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 5, 2021 •
edited
Loading

Deploy preview created for package "Web Components (Codesandbox Examples)":
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7314/index.html

Built with commit: 84318d62c05f30c45bb0b18f1e76b3c06efe9fbd

@guilhermelMoraes guilhermelMoraes added the Needs design approval PRs on feature requests and new components have to get design approval before merge. label Oct 5, 2021
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Oct 5, 2021 •
edited
Loading

Deploy preview created for package "React (Codesandbox Examples)":
https://react-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7314/index.html

Built with commit: 84318d62c05f30c45bb0b18f1e76b3c06efe9fbd

@skuruvil
Copy link

skuruvil commented Oct 6, 2021

@guilhermelMoraes As per design spec at max width both Intro section and pictogram item extend only till 7 column (Max-width: 640px). But current SB previews, I can see the pictogram item extends to 8 columns. Rest all looks good to me. CC @shixiedesign @DragosRistici

Screenshot 2021-10-06 at 10 58 02 PM

@oliviaflory
Copy link
Contributor

Hey @guilhermelMoraes, the content group pictogram changes look good! Seems aligned to the grid and everything.

Percy is picking up a lot of changes across different stories, like dotcom shell, tabs extended and footer, that don't have content group pictograms in them. Do you have any idea what might be causing the Percy changes?

@oliviaflory oliviaflory self-assigned this Oct 19, 2021
@oliviaflory
Copy link
Contributor

Thanks for the updates @guilhermelMoraes, Percy snapshot looks much better :D

Just a few small things:

  1. There's a moment in Web components where the pictogram items and content group sort of float off the grid between ~800px – 1055px.
    pictogram item Oct-19-2021 16-30-41

I believe it should look like the React example, where the Content group and pictogram align to the first column, and the pictogram item text is aligned to the 3rd column.
I'm not sure if this needs to be corrected in the pictogram item level or if this is just a storybook display issue.
Screen Shot 2021-10-19 at 4 31 53 PM

  1. In React Content block mixed (default and with aside elements) the pictogram group became inset by a 16px padding, so it's not quite aligned with the other content within the component
    Screen Shot 2021-10-19 at 4 39 39 PM

Screen Shot 2021-10-19 at 4 37 50 PM

@oliviaflory
Copy link
Contributor

hey @guilhermelMoraes or @BrunnoM7 it looks like the React build failed, could either of you kick it off again?

I checked the web components preview and it looks good!

I'm still seeing the inset comment I made (2.) about the alignment in React content block mixed on the Percy snapshot.

@oliviaflory oliviaflory mentioned this pull request Nov 2, 2021
Closed
@jeffchew
Copy link
Member

jeffchew commented Nov 4, 2021

Sorry @oliviaflory , just saw your note! I just ended up updating the branch to kick off all jobs again, hope it goes through this time!

@oliviaflory
Copy link
Contributor

Thanks @jeffchew for fixing the builds!

In React I'm still seeing the shift of the Content group pictograms within the Content block mixed and Content block mixed with aside elements in Storybook and Percy at all breakpoints. If we can get that fixed this will be good to go!
Screen Shot 2021-11-04 at 4 47 30 PM

Screen Shot 2021-11-04 at 4 51 08 PM

Screen Shot 2021-11-04 at 4 48 06 PM

@guilhermelMoraes
Merge branch 'master' of github.com:carbon-design-system/carbon-for-i…
95bedb9 
…bm-dotcom into fix/content-group-pictogram-spacing
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2021 •
edited
Loading

DCO Assistant Lite bot All Contributors have signed the CLA.

@guilhermelMoraes
Copy link
Contributor Author

I have read the DCO document and I hereby sign the DCO.

@guilhermelMoraes
Copy link
Contributor Author

recheck

@oliviaflory
Copy link
Contributor

Hey @guilhermelMoraes I am still seeing the inset content group pictograms in the Content block, so the alignment on the grid isn't quite right. See comment above for the image reference.

I'm not sure what could be causing it, any ideas?

@shixiedesign
Copy link
Contributor

Hey @guilhermelMoraes, Web components look good! Trying to see what's left to do on this one. Like @oliviaflory said, only issue I see is a regression in React. I know you might still be working on it, just wanna mention it to you. Let us know when you'd like another look.

image

oliviaflory
oliviaflory approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@oliviaflory oliviaflory left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @guilhermelMoraes for all your hard work!

@guilhermelMoraes guilhermelMoraes added package: web components Work necessary for the IBM.com Library web components package and removed Needs design approval PRs on feature requests and new components have to get design approval before merge. 🛠️ fix needed labels Nov 18, 2021
@oliviaflory oliviaflory mentioned this pull request Nov 18, 2021
Closed
BrunnoM7
BrunnoM7 approved these changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@BrunnoM7 BrunnoM7 left a comment

Choose a reason for hiding this comment

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

LGTM

@guilhermelMoraes guilhermelMoraes added the Ready to merge Label for the pull requests that are ready to merge label Nov 18, 2021
@kodiakhq kodiakhq bot merged commit fa146b1 into carbon-design-system:master Nov 18, 2021
@guilhermelMoraes guilhermelMoraes deleted the fix/content-group-pictogram-spacing branch November 18, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviaflory oliviaflory oliviaflory approved these changes

@BrunnoM7 BrunnoM7 BrunnoM7 approved these changes

@annawen1 annawen1 Awaiting requested review from annawen1 annawen1 was automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@skuruvil skuruvil Awaiting requested review from skuruvil skuruvil was automatically assigned from carbon-design-system/carbon-for-ibm-com-members

Assignees

@oliviaflory oliviaflory

Labels
package: web components Work necessary for the IBM.com Library web components package 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.
7 participants
@guilhermelMoraes @ibmdotcom-bot @skuruvil @oliviaflory @jeffchew @shixiedesign @BrunnoM7