-
Notifications
You must be signed in to change notification settings - Fork 529
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
AvatarStack no longer accepts styled props #1468
Conversation
🦋 Changeset detectedLatest commit: 494c752 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
docs/content/AvatarStack.mdx
Outdated
| Name | Type | Default | Description | | ||
| :--------- | :---------------- | :-----: | :----------------------------------- | | ||
| alignRight | Boolean | | Creates right aligned AvatarStack | | ||
| children | React.Node | | The avatars | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: Do we typically include children
(since every? React component supports it)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem like we generally do. Would it be useful to include it in places where we have an expectation of a certain kind of component? e.g.,:
| children | React.Node | | Generally an `Avatar` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the <Props>
component work for this component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question — I'll check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, will switch over to that.
If we want to get rid of styled props, but we still want to support A contrived example: <AvatarStack as="a" href="/foo">{...}</AvatarStack> Edit: @colebemis reminded me of this discussion, and I think AvatarStack does not merit an |
c726fc2
to
42be1ba
Compare
const count = React.Children.count(children) | ||
const wrapperClassNames = classnames({ | ||
'pc-AvatarStack--two': count === 2, | ||
'pc-AvatarStack--three-plus': count > 2, | ||
'pc-AvatarStack--right': alignRight | ||
}) | ||
return ( | ||
<AvatarStackWrapper count={count} className={wrapperClassNames} {...rest}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change prevents AvatarStack from accepting any DOM props (e.g. id
, className
, data-*
). I would love for us to document how we decide when a component should accept DOM props. Maybe it's appropriate for some components to accept all DOM props (e.g. Link
) and other components should only accept a subset of DOM props.
We should also make it obvious in our component documentation pages if a component accepts DOM props or not.
This is a bigger discussion that we can probably have outside of this PR.
For now, I'm fine with moving forward with this PR as-is. It might help us expose places where DOM props are necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call to make it clear in docs.
Proposal: The set of components that take an as
prop should be the same set that pass through DOM props. (I can't see any use case where we'd want those sets to diverge).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable to me. I'm trying to think of components that might not accept the as
prop but should still accept DOM props. What about form components? (like TextInput
)
How strange... it's working locally. I'll investigate! |
Co-authored-by: Cole Bemis <colebemis@github.com>
df51182
to
8638f60
Compare
title: AvatarStack | ||
--- | ||
|
||
import {Props} from '../src/props' | ||
import {AvatarStack} from '@primer/components' | ||
|
||
AvatarStack is used to display more than one Avatar in an inline stack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary for this PR, but while we're here it might be nice to add some more metadata to the docs:
title: AvatarStack | |
--- | |
import {Props} from '../src/props' | |
import {AvatarStack} from '@primer/components' | |
AvatarStack is used to display more than one Avatar in an inline stack. | |
title: AvatarStack | |
status: Alpha | |
source: https://github.com/primer/react/blob/main/src/AvatarStack.tsx | |
description: Use avatar stack to display multiple avatars in an inline stack. | |
--- | |
import {Props} from '../src/props' | |
import {AvatarStack} from '@primer/components' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 I would like that very much. I'm happy to work toward that in a separate stream of work.
See https://github.com/github/primer/issues/296
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.