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.

Sign up for GitHub

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

Jump to bottom

Enable responsive syntax for all of <Box>'s style props #10890

Closed
wants to merge 1 commit into from
Closed

Enable responsive syntax for all of <Box>'s style props #10890

wants to merge 1 commit into from

Conversation

jesstelford
Copy link
Contributor

@jesstelford jesstelford commented Oct 5, 2023 •
edited
Loading

Depends on #10489

This PR:

@jesstelford jesstelford changed the base branch from next to grid-responsive-props-10484 October 5, 2023 07:39
@jesstelford jesstelford mentioned this pull request Oct 5, 2023
Closed
@jesstelford jesstelford changed the title responsive props not use breakpoint Enable responsive syntax for all of <Box>'s style props Oct 5, 2023
@jesstelford jesstelford force-pushed the responsive-props-not-use-breakpoint branch 3 times, most recently from e777305 to acb1bcc Compare October 5, 2023 09:59
aaronccasanova
aaronccasanova reviewed Oct 5, 2023
View reviewed changes
polaris-react/src/components/Box/Box.scss
Comment on lines +13 to +36
@include responsive-props('box', 'background', 'background-color');
@include responsive-props('box', 'border-block-end-width', 'border-block-end-width', ('xs': 0));
@include responsive-props('box', 'border-block-start-width', 'border-block-start-width', ('xs': 0));
@include responsive-props('box', 'border-color', 'border-color');
@include responsive-props('box', 'border-end-end-radius', 'border-end-end-radius');
@include responsive-props('box', 'border-end-start-radius', 'border-end-start-radius');
@include responsive-props('box', 'border-inline-end-width', 'border-inline-end-width', ('xs': 0));
@include responsive-props('box', 'border-inline-start-width', 'border-inline-start-width', ('xs': 0));
@include responsive-props('box', 'border-start-end-radius', 'border-start-end-radius');
@include responsive-props('box', 'border-start-start-radius', 'border-start-start-radius');
@include responsive-props('box', 'border-style', 'border-style');
@include responsive-props('box', 'color', 'color');
@include responsive-props('box', 'inset-block-end', 'inset-block-end');
@include responsive-props('box', 'inset-block-start', 'inset-block-start');
@include responsive-props('box', 'inset-inline-end', 'inset-inline-end');
@include responsive-props('box', 'inset-inline-start', 'inset-inline-start');
@include responsive-props('box', 'max-width', 'max-width');
@include responsive-props('box', 'min-height', 'min-height');
@include responsive-props('box', 'min-width', 'min-width');
@include responsive-props('box', 'outline-color', 'outline-color');
@include responsive-props('box', 'outline-style', 'outline-style');
@include responsive-props('box', 'outline-width', 'outline-width');
@include responsive-props('box', 'overflow-x', 'overflow-x');
@include responsive-props('box', 'overflow-y', 'overflow-y');
Copy link
Member

@aaronccasanova aaronccasanova Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm all for this update! Might be a good opportunity to revisit my comment on the original implementation. Thoughts?

#7697 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooh, yes, love it! Let's get that into a fast-follow PR 👍

There's also an opportunity to optimize it even further by not even outputting a media query if that value is just going to fallback to an earlier breakpoint's value anyway.

@jesstelford jesstelford force-pushed the grid-responsive-props-10484 branch 2 times, most recently from 9ede084 to aa81906 Compare October 6, 2023 05:58
@jesstelford jesstelford force-pushed the responsive-props-not-use-breakpoint branch from acb1bcc to 3cbf209 Compare October 6, 2023 06:03
laurkim
laurkim reviewed Oct 6, 2023
View reviewed changes
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.

Looks great overall! 💯
Noticing some regressions with text when running locally though where the color isn't being set properly. That might be what's causing the large amount of baseline changes that need to be accepted for Storybook.

@jesstelford jesstelford mentioned this pull request Oct 7, 2023
Closed
@jesstelford jesstelford force-pushed the responsive-props-not-use-breakpoint branch from 3cbf209 to e274a87 Compare October 7, 2023 07:55
@jesstelford
Copy link
Contributor Author

jesstelford commented Oct 7, 2023 •
edited
Loading

@laurkim Good catch! I realised it was to do with initial vs inherit, and fixed that issue in #10489 (comment) (along with an explanation of the fix), then rebased this branch on top so it inherits that fix 👍

Will watch to see if Chromatic shows fewer changes now 🤞

EDIT: Yep, that worked 🎉 The "19 changes" showing at the moment are from the base branch. See more details here.

@jesstelford jesstelford force-pushed the responsive-props-not-use-breakpoint branch from e274a87 to 551e9c5 Compare October 7, 2023 08:38
@jesstelford jesstelford force-pushed the responsive-props-not-use-breakpoint branch from 551e9c5 to 68a3864 Compare October 7, 2023 21:53
@jesstelford jesstelford force-pushed the responsive-props-not-use-breakpoint branch from 68a3864 to 731328a Compare October 19, 2023 20:32
@sam-b-rose sam-b-rose mentioned this pull request Oct 23, 2023
Closed
@github-actions GitHub Actions
Copy link
Contributor

Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically.

If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.

@aaronccasanova aaronccasanova force-pushed the responsive-props-not-use-breakpoint branch from 731328a to f3a65b2 Compare April 19, 2024 21:52
@github-actions github-actions bot closed this May 20, 2024
@jesstelford jesstelford reopened this May 20, 2024
laurkim
laurkim approved these changes May 20, 2024
View reviewed changes
polaris-react/src/utilities/css.ts
responsiveProp: ResponsiveProp<Input> | undefined,
fn: (value?: Input) => Output | undefined,
): ResponsiveProp<Output> | undefined {
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the commented out code on lines 177-182 and 219-227?

@github-actions github-actions bot closed this Jun 20, 2024
@jesstelford jesstelford reopened this Jul 4, 2024
@github-actions github-actions bot closed this Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurkim laurkim laurkim approved these changes

@aaronccasanova aaronccasanova Awaiting requested review from aaronccasanova

Assignees
No one assigned
Labels
no-pr-activity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jesstelford @laurkim @aaronccasanova