contentful / forma-36

A design system by Contentful
https://f36.contentful.com
MIT License
331 stars 81 forks source link

feat(header): only apply margin left if noWrap is not first child #2772

Closed stephanLeece closed 3 months ago

stephanLeece commented 3 months ago

Purpose of PR

Left margin on Header’s title prop is no longer rendered when there’s no element to the left of the title

Screenshot 2024-06-03 at 12 31 06

Screenshot 2024-06-03 at 12 31 23

I updated the noWrap class styling so that the margin is only applied if the element is not first-child. This follows the existing approach we use for the title class

I had done something similar when working on the Updated Layout component, in this commit.

Tickets: https://contentful.atlassian.net/browse/CFISO-1571

PR Checklist

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
forma-36 ✅ Ready (Inspect) Visit Preview Jun 4, 2024 11:23am
changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: f5b4bafd1d00e3b6477c9fd7160e11082a58b4d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 40 packages | Name | Type | | ---------------------------- | ----- | | @contentful/f36-header | Major | | @contentful/f36-accordion | Major | | @contentful/f36-asset | Major | | @contentful/f36-autocomplete | Major | | @contentful/f36-badge | Major | | @contentful/f36-button | Major | | @contentful/f36-card | Major | | @contentful/f36-collapse | Major | | @contentful/f36-copybutton | Major | | @contentful/f36-core | Major | | @contentful/f36-datetime | Major | | @contentful/f36-datepicker | Major | | @contentful/f36-drag-handle | Major | | @contentful/f36-entity-list | Major | | @contentful/f36-empty-state | Major | | @contentful/f36-forms | Major | | @contentful/f36-icon | Major | | @contentful/f36-list | Major | | @contentful/f36-menu | Major | | @contentful/f36-modal | Major | | @contentful/f36-navbar | Major | | @contentful/f36-note | Major | | @contentful/f36-notification | Major | | @contentful/f36-pagination | Major | | @contentful/f36-pill | Major | | @contentful/f36-popover | Major | | @contentful/f36-skeleton | Major | | @contentful/f36-spinner | Major | | @contentful/f36-table | Major | | @contentful/f36-tabs | Major | | @contentful/f36-text-link | Major | | @contentful/f36-tooltip | Major | | @contentful/f36-typography | Major | | @contentful/f36-components | Major | | @contentful/f36-image | Major | | @contentful/f36-avatar | Major | | @contentful/f36-multiselect | Patch | | @contentful/f36-workbench | Patch | | @contentful/f36-icons | Patch | | @contentful/f36-docs-utils | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 3 months ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
CommonJS 134.77 KB (0%) 2.7 s (0%) 98 ms (+61.46% 🔺) 2.8 s
Module 131.19 KB (0%) 2.7 s (0%) 101 ms (+35.54% 🔺) 2.8 s
stephanLeece commented 3 months ago

Let me know what the release process for this should be. Would the next Header package version be "4.0.0-alpha.8"?

I assume it should still be merged into main, should that happen before or after the new Header package version is published?

massao commented 3 months ago

Let me know what the release process for this should be. Would the next Header package version be "4.0.0-alpha.8"?

I assume it should still be merged into main, should that happen before or after the new Header package version is published?

Yes, the version should be 4.0.0-alpha.8, and you should publish it before merging, since it needs to have the version on the package.json updated.

For release, since we don't have setup for releasing prereleases automatically, you would need to release it manually, there is a npm run prerelease script that should help with it, just remember to run npm run build before the release, to avoid releasing the version with the old build

stephanLeece commented 3 months ago

ughh.. published as 4.0.0-alpha.9 due to connection issues and me being too quick to hammer the retry button without reverting the change in package.json first.

If that's a big no no, is it possible to unpublish and then republish as the correct version number?

massao commented 3 months ago

ughh.. published as 4.0.0-alpha.9 due to connection issues and me being too quick to hammer the retry button without reverting the change in package.json first.

If that's a big no no, is it possible to unpublish and then republish as the correct version number?

No worries, specially being alpha version, we just skipped the .8 😄

stephanLeece commented 3 months ago

Do you want to bring this component out of alpha for v4?

Sure, would we move it to beta? so 4.0.0-beta.0?

denkristoffer commented 3 months ago

Sure, would we move it to beta? so 4.0.0-beta.0?

No just a stable component at this point. I don't think we are going to do much more with it for v4.

stephanLeece commented 3 months ago

Sounds good, anything i need to keep in mind when moving a component to a stable release? I guess:

denkristoffer commented 3 months ago

@stephanLeece We need docs on the website, but we have that: https://forma-36-git-cfiso-1571-update-header-left-margin.colorfuldemo.com/components/header – the "Alpha" badge should be removed though. Your list is correct, but you should also add it to the array with fixed versions in the changeset config. Regarding the version, you can set it to 4.0.0 and the release script should do the rest.

denkristoffer commented 3 months ago

@allcontributors add @stephanLeece for code, maintenance

allcontributors[bot] commented 3 months ago

@denkristoffer

I've put up a pull request to add @stephanLeece! :tada:

stephanLeece commented 3 months ago

@stephanLeece We need docs on the website, but we have that: https://forma-36-git-cfiso-1571-update-header-left-margin.colorfuldemo.com/components/header – the "Alpha" badge should be removed though. Your list is correct, but you should also add it to the array with fixed versions in the changeset config. Regarding the version, you can set it to 4.0.0 and the release script should do the rest.

Done! If all looks good now, I'll go ahead and merge :)

massao commented 3 months ago

Do we want to add this under the f36-components package?? We can do it later anyway

Also need to update the imports on Website and Sandpack.

stephanLeece commented 3 months ago

Closing this due to circleCi issues, will reopen with requested changes