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

Remove external/outside margin from components #9830

Merged
merged 13 commits into from
Oct 19, 2021

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Oct 8, 2021

Closes #5367
Refs #9805

Removes external/outside "harmful margin" from components listed in the original issue. The changes are only within @carbon/styles because these are considered breaking changes and should only be used with v11.

Changelog

Removed

  • form: remove fieldset, fileuploader, label margin
  • notifications: remove external margin from inline, toast, and actionable notification
  • structured-list: remove external margin

Testing / Reviewing

  • Existing styles for these components should remain unchanged in the carbon-components-react storybook
  • Styles for these components should be updated within the carbon-react-next storybook

@tay1orjones tay1orjones requested a review from a team as a code owner October 8, 2021 21:41
@netlify
Copy link

netlify bot commented Oct 8, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 759acf0

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/616f14fa7d95ed000819f8f7

😎 Browse the preview: https://deploy-preview-9830--carbon-react-next.netlify.app

@tay1orjones tay1orjones changed the title 5367 remove margin Remove external/outside margin from components Oct 8, 2021
@tay1orjones
Copy link
Member Author

A few components listed in the original issue have already had margin removed or were fixed to no longer include margin:

  • InlineLoading
  • Loading
  • Toggle
  • SmallToggle

cc @janhassel

@netlify
Copy link

netlify bot commented Oct 8, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 759acf0

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/616f14faa6a1b00007099399

😎 Browse the preview: https://deploy-preview-9830--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 8, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 759acf0

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/616f14fab65fba00070ee6a3

😎 Browse the preview: https://deploy-preview-9830--carbon-elements.netlify.app

@vpicone
Copy link
Contributor

vpicone commented Oct 9, 2021

Do we need to document these somewhere? Could see it being a surprise under some circumstances. re: feature flag, I think that's a design decision we have to make (or maybe have already made? @joshblack). Is the goal for folks to be able to switch from carbon-components to @carbon/styles as a drop-in change without needing to modify their code?

I could see the benefit of that as far as the adoption of the new package is concerned.

@tay1orjones
Copy link
Member Author

@vpicone Yeah, great points. I'll add it to the migration doc and put this behind the feature flag.

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Looks good to me 🏄🏾

@tay1orjones
Copy link
Member Author

Update on this - the storybooks should both remain unchanged as they both have the v11 feature flag turned off. To validate the margin removal, you have to pull down locally and add turn on the v11 feature flag.

@tay1orjones
Copy link
Member Author

@sstrubberg I added a commit just now passing the CARBON_ENABLE_V11_RELEASE environment variable to the sass compilation for the carbon-components-react storybook. So if you pull down locally and run CARBON_ENABLE_V11_RELEASE=true yarn storybook in packages/react you'll see the margin no longer included.

@sstrubberg sstrubberg enabled auto-merge (squash) October 19, 2021 18:56
@sstrubberg sstrubberg merged commit 7273119 into carbon-design-system:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify default component spacings
5 participants