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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Banner mobile styles #11959

Conversation

lgriffee
Copy link
Member

@lgriffee lgriffee commented Apr 29, 2024

WHY are these changes introduced?

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

WHAT is this pull request doing?

  • Removes shadow below sm breakpoint
  • Changes styling of banners with titles to match that of InlineIconBanners below sm breakpoint
  • Tightens spacing between of action buttons below sm breakpoint
  • Makes action buttons take up available space below sm breakpoint
Before (below sm breakpoint) After (below sm breakpoint)
5d559397bae39100201eedc1-dopnorbvqa chromatic com_iframe html_args= globals=viewport_0 id=all-components-banner--all viewMode=story(iPhone SE) 5d559397bae39100201eedc1-wsfhiihlki chromatic com_iframe html_args= globals=viewport_0 id=all-components-banner--all viewMode=story(iPhone SE)

@lgriffee lgriffee added the #gsd:40396 WebView infrastructure - Forms and Polaris label Apr 29, 2024
@lgriffee lgriffee self-assigned this Apr 29, 2024
@lgriffee lgriffee requested a review from ssetem April 29, 2024 21:31
@lgriffee lgriffee changed the title Update Banner mobile styles [Prototype] Update Banner mobile styles Apr 29, 2024
@lgriffee lgriffee mentioned this pull request Apr 29, 2024
Comment on lines +302 to +303
action={{content: 'Primary'}}
secondaryAction={{content: 'Secondary'}}
Copy link
Member Author

Choose a reason for hiding this comment

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

Made this change so we can see some examples of buttons side by side instead of stacked on small screens

@lgriffee lgriffee changed the title [Prototype] Update Banner mobile styles Update Banner mobile styles May 7, 2024
@lgriffee lgriffee requested a review from aveline May 7, 2024 22:30
@lgriffee lgriffee marked this pull request as ready for review May 7, 2024 22:33
@davebcn87
Copy link

davebcn87 commented May 8, 2024

I think we are missing some alignment between the icon and the title. Also, is the title the proper size? feels a bit small to me. Most likely it is because the mobile typography is not yet applied, right?

Screenshot 2024-05-08 at 12 22 37

@lgriffee lgriffee changed the base branch from main to revert-11833-revert-11724-theme-to-responsive May 8, 2024 15:20
@lgriffee
Copy link
Member Author

lgriffee commented May 8, 2024

I think we are missing some alignment between the icon and the title. Also, is the title the proper size? feels a bit small to me. Most likely it is because the mobile typography is not yet applied, right?

@davebcn87 Good callout! Definitely agree about the alignment there. I hadn't originally based this PR off the typography updates but your right that it's an important factor in reviewing all the alignment. I've updated the PR do that now (see screenshots above in the PR description), but now the body text looks giant. Not sure if we will need to adjust the Banner font sizes to compensate? Might be worth it for me to book some time with @heyjoethomas to figure out how we want to handle that (or to just bring it up during our Banner meeting tomorrow). In the meantime I'll work on aligning the title better for the InlineIconBanners.

@lgriffee
Copy link
Member Author

lgriffee commented May 8, 2024

@davebcn87 I've updated the title alignment on the InlineIconBanners in 1f08404
Screenshot 2024-05-08 at 9 38 45鈥疉M

@lgriffee lgriffee closed this May 9, 2024
@lgriffee
Copy link
Member Author

lgriffee commented May 9, 2024

Closing in favor of build phase PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:40396 WebView infrastructure - Forms and Polaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants