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

docs(spinner): add example showcasing $as prop #3111

Merged
merged 3 commits into from Mar 24, 2020
Merged

Conversation

sandgraham
Copy link
Contributor

Adds a new example to the Spinner docs to show how you can manipulate the underlying StyledSpinnerNext element.

@vercel
Copy link

vercel bot commented Mar 24, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/uber-ui-platform/baseweb/j0g8gsjm9
✅ Preview: https://baseweb-git-docs-spinner-span.uber-ui-platform.now.sh


export default () => (
<p>
<StyledSpinnerNext $as="span" $style={{display: 'block'}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd incline not to use $style in the examples. Can we replace it with withStyle?

Copy link
Contributor

Choose a reason for hiding this comment

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

just out of curiosity, what's wrong with $style? we document it on the styletron website (https://www.styletron.org/react/#style-prop), so I think one would assume it's part of the public API?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that we document it in Styletron docs, but it's not a part of the base web API we've been encouraging people to use. I also think it may create more confusion around how and when one would use it. It's more of an internal implementation imho.

<SpinnerNextSpan />
</Example>

By default the spinner styles a single `div` element. You can change the underlying element type by using the `$as` prop. In this example we change it to a `span` and add `display: block`. This allows us to use the `Spinner` inside of a `p` element. Note, we could also set `display: inline-block` to prevent the spinner from wrapping to a new line.
Copy link
Contributor

Choose a reason for hiding this comment

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

While helping someone who brought up the issue mentioned here ^ I also notices that the API for this styled element is different from the current Spinner component but we didn't document it anywhere. Could you add it here as well?

@sandgraham
Copy link
Contributor Author

sandgraham commented Mar 24, 2020

@nadiia I changed this up a bit. I added display: block to the default styling, which shouldn't be breaking since it is already div default, but lets you change to span without having to set the styles. Also updated some of the text in this section to make it easier to read (imo).

Also considering if the default should be span with display: block.

@sandgraham sandgraham merged commit b4f72fd into master Mar 24, 2020
@uber-baseweb-probots uber-baseweb-probots bot deleted the docs-spinner-span branch March 24, 2020 21:26
cbranch101 pushed a commit to cbranch101/baseweb that referenced this pull request Mar 26, 2020
* docs(spinner): add example showcasing $as prop

* feat(spinner): add explicit display property

* docs(spinner): rework examples
VladimirMilenko pushed a commit to VladimirMilenko/baseui that referenced this pull request Apr 2, 2020
* docs(spinner): add example showcasing $as prop

* feat(spinner): add explicit display property

* docs(spinner): rework examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants