Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

Conversation

kyledurand
Copy link
Member

No description provided.

@kyledurand kyledurand self-assigned this Jan 5, 2023
@kyledurand
Copy link
Member Author

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

size-limit report 📦

Path Size
polaris-react-cjs 211.52 KB (-0.01% 🔽)
polaris-react-esm 136.62 KB (-0.01% 🔽)
polaris-react-esnext 191.86 KB (-0.01% 🔽)
polaris-react-css 41.76 KB (0%)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

🫰✨ Thanks @kyledurand! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20230105142108
yarn add @shopify/polaris@0.0.0-snapshot-release-20230105142108

@kyledurand kyledurand requested review from sarahill and aveline January 5, 2023 14:50
@aveline
Copy link
Contributor

aveline commented Jan 5, 2023

Prod Spin
Screenshot 2023-01-05 at 7 42 39 AM Screenshot 2023-01-05 at 7 39 32 AM

The top padding is still creating extra spacing 🤔

@kyledurand
Copy link
Member Author

Can you link to that page @aveline? Can you tell if it's another instance of margin collapse or is it that the children aren't getting rendered in production

@kyledurand
Copy link
Member Author

kyledurand commented Jan 5, 2023

I found it after clicking around on the discount page. I think we'll need to not render children if they're not present in this case since it is margin collapse. Wonder if that could cause issues though

Edit I think the above is an implementation issue. Children shouldn't be rendered unless that option is selected. I'm creating a better example to showcase this

@aveline
Copy link
Contributor

aveline commented Jan 5, 2023

Yes, worth trying though and checking in web.

Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

✅ LGTM

Some things I think we can tighten up in a separate issue but this matches what I'm seeing in prod.

Copy link
Contributor

@aveline aveline left a comment

Choose a reason for hiding this comment

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

Looks like it renders as expected when implemented correctly and empty children aren't rendered.

@kyledurand
Copy link
Member Author

Or, a bit hacky we could make the padding conditional on renderedChildren

renderChildren is a function with a return value in the case of what's going on in web so there's really no way for us to check that here

@kyledurand kyledurand merged commit 382784f into main Jan 6, 2023
@kyledurand kyledurand deleted the choicelist_fix-spacing branch January 6, 2023 15:09
kyledurand pushed a commit that referenced this pull request Jan 6, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris@10.18.0

### Minor Changes

- [#7992](#7992)
[`e8f74f4cd`](e8f74f4)
Thanks [@aveline](https://github.com/aveline)! - Added support for
`outline` to `Box`

### Patch Changes

- [#7988](#7988)
[`382784f4e`](382784f)
Thanks [@kyledurand](https://github.com/kyledurand)! - Reduced spacing
on ChoiceList children


- [#7899](#7899)
[`930f077eb`](930f077)
Thanks [@jeradg](https://github.com/jeradg)! - Fixed a bug where
Tooltips nested in Scrollable containers sometimes don't update their
positions correctly


- [#7831](#7831)
[`47487ee0c`](47487ee)
Thanks [@acmertz](https://github.com/acmertz)! - Updated the focus
helper functions to no longer treat buttons with `aria-disabled="true"`
and `tabindex="-1" (but no`disabled\` attribute) as focusable.

## @shopify/plugin-polaris@0.0.25



## polaris.shopify.com@0.28.2

### Patch Changes

- Updated dependencies
\[[`382784f4e`](382784f),
[`930f077eb`](930f077),
[`47487ee0c`](47487ee),
[`e8f74f4cd`](e8f74f4)]:
    -   @shopify/polaris@10.18.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@gwyneplaine gwyneplaine mentioned this pull request Feb 14, 2023
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris@10.18.0

### Minor Changes

- [Shopify#7992](Shopify#7992)
[`e8f74f4cd`](Shopify@e8f74f4)
Thanks [@aveline](https://github.com/aveline)! - Added support for
`outline` to `Box`

### Patch Changes

- [Shopify#7988](Shopify#7988)
[`382784f4e`](Shopify@382784f)
Thanks [@kyledurand](https://github.com/kyledurand)! - Reduced spacing
on ChoiceList children


- [Shopify#7899](Shopify#7899)
[`930f077eb`](Shopify@930f077)
Thanks [@jeradg](https://github.com/jeradg)! - Fixed a bug where
Tooltips nested in Scrollable containers sometimes don't update their
positions correctly


- [Shopify#7831](Shopify#7831)
[`47487ee0c`](Shopify@47487ee)
Thanks [@acmertz](https://github.com/acmertz)! - Updated the focus
helper functions to no longer treat buttons with `aria-disabled="true"`
and `tabindex="-1" (but no`disabled\` attribute) as focusable.

## @shopify/plugin-polaris@0.0.25



## polaris.shopify.com@0.28.2

### Patch Changes

- Updated dependencies
\[[`382784f4e`](Shopify@382784f),
[`930f077eb`](Shopify@930f077),
[`47487ee0c`](Shopify@47487ee),
[`e8f74f4cd`](Shopify@e8f74f4)]:
    -   @shopify/polaris@10.18.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants