search

artsy / palette-mobile

Artsy's Design System on Mobile
21 stars 2 forks source link

fix: Align right header element correctly #200

Closed olerichter00 closed 8 months ago

olerichter00 commented 8 months ago

Resolves https://www.notion.so/artsy/Manage-saves-font-looks-broken-on-the-offer-page-update-font-size-according-to-design-and-center-bu-b344bd5a99f941fea5df5d4048679090?pvs=4

Description

This pull request corrects the text alignment and centering in the screen header component.

Before After
Simulator Screenshot - iPhone 15 Pro - 2024-03-04 at 16 10 10 Simulator Screenshot - iPhone 15 Pro - 2024-03-04 at 15 55 22
📦 Published PR as canary version: 13.1.18--canary.200.1559.0
:sparkles: Test out this PR locally via: ```bash npm install @artsy/palette-mobile@13.1.18--canary.200.1559.0 # or yarn add @artsy/palette-mobile@13.1.18--canary.200.1559.0 ```
olerichter00 commented 8 months ago

I have a sneaking suspicion that this change might break Folio, which has a different manner of of title behavior than eigen (mostly left aligned, up to the width of the left element). Can we achieve the same thing by remaining with flexbox? As soon as things become manually positioned we lose all manner of flexibility. Also, not sure i'm seeing the issue in the screenshots.

Thanks for the reviews 🙏

Before this fix, we had to set the width of the right element manually to ensure it is rendered correctly (Eigen code).

I just realized that what I'm trying to achieve with position: absolute would also work by setting flex: 1 for both the left and right elements to ensure they have the same size, positioning the title exactly in the center of the screen (JSFiddle with CSS example)💡 I'll update this PR and get rid of the position: absolute.

damassi commented 8 months ago

Sweet, that seems like a good fix!

olerichter00 commented 8 months ago

Closing this PR in favor of https://github.com/artsy/palette-mobile/pull/203