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

Update component docs (AnchoredOverlay, Avatar, AvatarStack, Box, BranchName, Breadcrumbs) #1702

Merged
merged 12 commits into from
Dec 7, 2021

Conversation

colebemis
Copy link
Contributor

@colebemis colebemis commented Dec 7, 2021

@colebemis colebemis requested review from a team and mperrotti December 7, 2021 00:48
@changeset-bot
Copy link

changeset-bot bot commented Dec 7, 2021

🦋 Changeset detected

Latest commit: 94b8ffe

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

This PR includes changesets to release 1 package
Name Type
@primer/components 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
Copy link
Contributor

github-actions bot commented Dec 7, 2021

size-limit report 📦

Path Size
dist/browser.esm.js 57.25 KB (+0.01% 🔺)
dist/browser.umd.js 57.61 KB (+0.02% 🔺)

Copy link
Contributor

@jfuchs jfuchs left a comment

Choose a reason for hiding this comment

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

So much better! 🎨 🥇

side note: It would be awesome if we could get syntax highlighting for our Type column in these tables.

name="overlayProps"
type={
<>
Partial&lt;<Link href="/Overlay#props">OverlayProps</Link>&gt;
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -6,9 +6,11 @@ exports[`BranchName renders consistently 1`] = `
padding: 2px 6px;
font-size: 12px;
font-family: SFMono-Regular,Consolas,"Liberation Mono",Menlo,Courier,monospace;
color: #57606a;
color: #0969da;
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a pretty drastic change — is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our current BranchName component is pretty different from what's on github.com:

github.com Primer React
CleanShot 2021-12-07 at 12 11 40 CleanShot 2021-12-07 at 12 10 41

This change fixes that.

<>
An override to the internal <InlineCode>renderAnchor</InlineCode> ref that will be used to position the overlay.
When <InlineCode>renderAnchor</InlineCode> is
<InlineCode>null</InlineCode>, this can be used to make an anchor that is detached from <InlineCode>
Copy link
Contributor

Choose a reason for hiding this comment

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

This wording is tricky — unlike most ref props, there's an expectation that the AnchoredOverlay isn't responsible for setting the value of the ref, right? only that it will use the ref's current value for positioning logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this one is tricky. I mostly copied this description from the code comments but I agree that it's a bit confusing.

<PropsTableRow
name="alt"
type="string"
defaultValue="''"
Copy link
Contributor

Choose a reason for hiding this comment

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

❗ I'm surprised this is '' and not undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is intentional. alt='' is treated differently than an undefined alt. From https://davidwalsh.name/accessibility-tip-empty-alt-attributes:

Images with only visual value should have an empty alt attribute set on them. Omitting the alt attribute makes most screen readers read out the entire image URL and providing an alt attribute when the image is for visual purposes only is just as useless.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL, thanks!

@colebemis colebemis merged commit 2c42c38 into main Dec 7, 2021
@colebemis colebemis deleted the update-component-docs-2 branch December 7, 2021 21:44
@github-actions github-actions bot mentioned this pull request Dec 7, 2021
pksjce pushed a commit that referenced this pull request Dec 20, 2021
…nchName, Breadcrumbs) (#1702)

* Update AnchoredOverlay docs

* Allow preformatted types in prop table

* Add Link and InlineCode to global mdx scope

* Update Avatar docs

* Update AvatarStack docs

* Update Box props

* Update branchname docs

* Update breadcrumbs docs

* Update branchname snapshot

* Create polite-trees-wink.md

* Add AvatarPair docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants