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

Conversation

kyledurand
Copy link
Member

@kyledurand kyledurand commented Nov 11, 2022

I'm still of the opinion that our layout components should have some defaults and the ones we have on bleed may not be right but we need to use it in more places to figure out what those defaults should be.

For now, this fixes regressions to Banner that were introduced in #7644

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
polaris-react-cjs 210.06 KB (+0.01% 🔺)
polaris-react-esm 136.18 KB (+0.01% 🔺)
polaris-react-esnext 190.7 KB (+0.01% 🔺)
polaris-react-css 41.17 KB (0%)

@kyledurand kyledurand marked this pull request as ready for review November 11, 2022 18:08
@kyledurand kyledurand changed the title Fix bleed regressions Fix Bleed regressions Nov 11, 2022
@kyledurand kyledurand self-assigned this Nov 16, 2022
@kyledurand kyledurand requested review from chazdean, laurkim and aveline and removed request for chazdean and laurkim November 16, 2022 19:27
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.

I still think it's an odd pattern to have to set ="0"s if the defaults don't suit. I think defaults are good if they get silently overridden.

If we still need to figure out the correct defaults, maybe better to leave them off for now? Once we get more usage of Bleed we can see what the most common values are and use that to inform the defaults.

@kyledurand kyledurand requested a review from laurkim November 17, 2022 14:22
@kyledurand
Copy link
Member Author

I don't see any point in holding off for now. We're going to ship these components with opinions. They may be wrong right now but we can change them in the future

Copy link
Contributor

@laurkim laurkim left a comment

Choose a reason for hiding this comment

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

I'm okay with this approach because I think it may fix broken bleed examples on the style guide as well. As we continue to rebuild internal components, we can discuss more in detail whether we want to keep the approach of adding props with the 0 value or remove it. Given that it's still in the alpha phase, I think we should ship this approach and continue iterating in subsequent PRs.

@kyledurand
Copy link
Member Author

Going to ship this but please keep sharing the pain points, it's how we learn and get better ❤️

@kyledurand kyledurand merged commit e1679bf into layout-components-beta Nov 17, 2022
@kyledurand kyledurand deleted the fix-bleed-regressions branch November 17, 2022 14:38
laurkim pushed a commit that referenced this pull request Nov 23, 2022
I'm still of the opinion that our layout components should have some
defaults and the ones we have on bleed may not be right but we need to
use it in more places to figure out what those defaults should be.

For now, this fixes regressions to Banner that were introduced in
#7644
chazdean pushed a commit that referenced this pull request Nov 28, 2022
I'm still of the opinion that our layout components should have some
defaults and the ones we have on bleed may not be right but we need to
use it in more places to figure out what those defaults should be.

For now, this fixes regressions to Banner that were introduced in
#7644
laurkim pushed a commit that referenced this pull request Dec 5, 2022
I'm still of the opinion that our layout components should have some
defaults and the ones we have on bleed may not be right but we need to
use it in more places to figure out what those defaults should be.

For now, this fixes regressions to Banner that were introduced in
#7644
laurkim pushed a commit that referenced this pull request Dec 7, 2022
I'm still of the opinion that our layout components should have some
defaults and the ones we have on bleed may not be right but we need to
use it in more places to figure out what those defaults should be.

For now, this fixes regressions to Banner that were introduced in
#7644
laurkim pushed a commit that referenced this pull request Dec 9, 2022
I'm still of the opinion that our layout components should have some
defaults and the ones we have on bleed may not be right but we need to
use it in more places to figure out what those defaults should be.

For now, this fixes regressions to Banner that were introduced in
#7644
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