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

Change Banner layout to better align with mobile #11887

Closed

Conversation

mauriciomeirelles
Copy link

@mauriciomeirelles mauriciomeirelles commented Apr 12, 2024

WHY are these changes introduced?

Fixes https://github.com/Shopify/mobile/issues/33410

WHAT is this pull request doing?

Implement design from https://www.figma.com/file/PQUFqE9VrOtVPZbaifdL5J/Forms?type=design&node-id=2241-9876&mode=design&t=IEI19PTpbkZdyhKO-0

Screenshot 2024-04-29 at 13 30 36

Also changes the success icon from CheckIcon to CheckCircleIcon

Screenshot 2024-04-12 at 17 12 40

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@mauriciomeirelles mauriciomeirelles changed the title Change Banner border radius on sm to be 300 instead of 0 Change Banner border radius on xs to be 300 instead of 0 Apr 12, 2024
@mauriciomeirelles mauriciomeirelles force-pushed the mmeirelles/change_banner_border_radius_sm branch from ef65185 to bb935f6 Compare April 12, 2024 07:57
@ssetem ssetem mentioned this pull request Apr 12, 2024
@mauriciomeirelles mauriciomeirelles force-pushed the mmeirelles/change_banner_border_radius_sm branch 2 times, most recently from 3f43430 to 0432570 Compare April 12, 2024 15:13
@mauriciomeirelles
Copy link
Author

/snapit

@mauriciomeirelles mauriciomeirelles self-assigned this Apr 12, 2024
Copy link
Contributor

🫰✨ Thanks @mauriciomeirelles! Your snapshothas been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240412151826"

@mauriciomeirelles mauriciomeirelles force-pushed the mmeirelles/change_banner_border_radius_sm branch 2 times, most recently from 7be79f2 to c2ad416 Compare April 12, 2024 15:41
@mauriciomeirelles mauriciomeirelles force-pushed the mmeirelles/change_banner_border_radius_sm branch from c2ad416 to c6ff816 Compare April 12, 2024 15:45
@mauriciomeirelles mauriciomeirelles marked this pull request as ready for review April 12, 2024 15:45
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Code and storybook look good. Can you share the spin link when you get a chance?

'@shopify/polaris': patch
---

Add border to Banner on breakpoint-xs and change success icon to CheckCircleIcon
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Add border to Banner on breakpoint-xs and change success icon to CheckCircleIcon
Added border to Banner on breakpoint-xs and change success icon to CheckCircleIcon

@@ -3,6 +3,12 @@
position: relative;
display: flex;

margin-inline: var(--p-space-300);
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 still have to support old mobile safari versions? I thought logical properties were still out because of that. Not a blocker at all. I'm all for using them since it's not task blocking for the long tail

@mauriciomeirelles
Copy link
Author

/snapit

Copy link
Contributor

🫰✨ Thanks @mauriciomeirelles! Your snapshothas been published to npm.

Test the snapshot by updating your package.json with the newly published version:

"@shopify/polaris": "0.0.0-snapshot-20240415094702"

@mauriciomeirelles mauriciomeirelles force-pushed the mmeirelles/change_banner_border_radius_sm branch 2 times, most recently from 7aca883 to 5c36a7b Compare April 16, 2024 07:51
Copy link
Contributor

@heyjoethomas heyjoethomas left a comment

Choose a reason for hiding this comment

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

Is this being applied to all versions of Banner? Also does this change make sure their is 16px of padding between the Banner and the edge of the screen?

@mauriciomeirelles mauriciomeirelles marked this pull request as draft April 18, 2024 14:17
@aaronccasanova aaronccasanova force-pushed the mmeirelles/change_banner_border_radius_sm branch from 5c36a7b to a299eb3 Compare April 19, 2024 21:52
@github-actions github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024
@mauriciomeirelles mauriciomeirelles force-pushed the mmeirelles/change_banner_border_radius_sm branch from a299eb3 to 15930a6 Compare April 29, 2024 11:25
@mauriciomeirelles mauriciomeirelles force-pushed the mmeirelles/change_banner_border_radius_sm branch from 15930a6 to bf15e36 Compare April 29, 2024 11:25
@github-actions github-actions bot removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 29, 2024
@mauriciomeirelles mauriciomeirelles changed the title Change Banner border radius on xs to be 300 instead of 0 Change Banner layout to better align with mobile Apr 29, 2024
@mauriciomeirelles
Copy link
Author

/snapit

Copy link
Contributor

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

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

"@shopify/polaris-migrator": "0.0.0-snapshot-20240429113201",
"@shopify/polaris": "0.0.0-snapshot-20240429113201",
"@shopify/polaris-tokens": "0.0.0-snapshot-20240429113201",
"@shopify/stylelint-polaris": "0.0.0-snapshot-20240429113201"

@lgriffee
Copy link
Member

Closing in favor of #11959 to only apply changes below sm breakpoint.

@lgriffee lgriffee closed this Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants