Skip to content

Bug fix: ActionList item label weight and spacing if description exists - #3490

Merged
langermank merged 5 commits into
mainfrom
fix-actionlist-item-bold
Jul 6, 2023
Merged

Bug fix: ActionList item label weight and spacing if description exists#3490
langermank merged 5 commits into
mainfrom
fix-actionlist-item-bold

Conversation

@langermank

@langermanklangermank commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

The item label should be bold is there's a description. I think this likely broke at some point recently. Also fixed the spacing between rows.

Screenshots

BeforeAfter
Action list with description before fixAction list with description after fix

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@langermank
langermank requested review from a team and josepmartinsJuly 6, 2023 03:01
@changeset-bot

changeset-botBot commented Jul 6, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 45d7b83

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

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

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

github-actionsBot commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js101.83 KB (+0.03% 🔺)
dist/browser.umd.js102.36 KB (+0.02% 🔺)

@github-actions
github-actionsBot temporarily deployed to storybook-preview-3490 July 6, 2023 03:08 Inactive
@langermank
langermank temporarily deployed to github-pages July 6, 2023 03:12 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3490 July 6, 2023 03:12 Inactive
fontWeight: slots.description && slots.description.props.variant !== 'block' ? 'bold' : 'normal',
fontWeight: slots.description ? 'bold' : 'normal',
marginBlockEnd:
slots.description && slots.description.props.variant !== 'inline' ? '4px' : undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this be the token space-1 than 4px?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good catch!

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Can someone let me know what the syntax is meant to look like to access the spacing token here? I couldn't figure it out, gave up and used px 😛

@siddharthkpsiddharthkpJul 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

- slots.description && slots.description.props.variant !== 'inline' ? '4px' : undefined,+ slots.description && slots.description.props.variant !== 'inline' ? 1 : undefined,

unlessssssss, styled-system doesn't understand logical properties like marginBlockEnd, only marginRight because the last release was in 2019 (open issue).

We could figure out how to add support for logical properties in primer/react or we could add '4px' for now and move on. I say 4px and move on. (only for primer, look the other way @maraisr 😛)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

SIGH haha okay, thanks @siddharthkp!

@maraisrmaraisr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you so much for this :shipit:

@josepmartinsjosepmartins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! 👍

@siddharthkpsiddharthkp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@langermank
langermank temporarily deployed to github-pages July 6, 2023 16:56 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3490 July 6, 2023 16:57 Inactive
@langermank
langermank temporarily deployed to github-pages July 6, 2023 18:52 — with GitHub Actions Inactive
@github-actions
github-actionsBot temporarily deployed to storybook-preview-3490 July 6, 2023 18:52 Inactive
@langermanklangermank added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jul 6, 2023
@github-actionsgithub-actionsBot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jul 6, 2023
@github-actions
github-actionsBottemporarily deployed to storybook-preview-3490 July 6, 2023 19:26 Inactive
@langermank
langermank added this pull request to the merge queueJul 6, 2023
Merged via the queue into main with commit 8d1d514Jul 6, 2023
@langermank
langermank deleted the fix-actionlist-item-bold branch July 6, 2023 20:18
@primer-cssprimer-css mentioned this pull request Jul 6, 2023
Sign up for freeto 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.

4 participants

@langermank@maraisr@josepmartins@siddharthkp