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

fix(ONYX-1375): fix SellWithArtsyRecentlySold Rail layout #11015

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

dariakoko
Copy link
Contributor

@dariakoko dariakoko commented Oct 24, 2024

This PR resolves ONYX-1375

Description

After the latest redesign and refactoring the UI of the SellWithArtsyRecentlySold Rail does not look correct.
Let’s adjust it to make the data on the artwork card assessable

Before After
image image

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • fix SellWithArtsyRecentlySold Rail layout - daria

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

@dariakoko dariakoko requested a review from a team October 24, 2024 17:14
@dariakoko dariakoko self-assigned this Oct 24, 2024
anandaroop
anandaroop previously approved these changes Oct 24, 2024
@dariakoko dariakoko marked this pull request as ready for review October 25, 2024 07:24
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Oct 25, 2024

This PR contains the following changes:

  • Cross-platform user-facing changes (fix SellWithArtsyRecentlySold Rail layout - daria - dariakoko)

Generated by 🚫 dangerJS against 8b290a6

olerichter00
olerichter00 previously approved these changes Oct 25, 2024
Copy link
Contributor

@olerichter00 olerichter00 left a comment

Choose a reason for hiding this comment

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

Thanks! 🙏

@@ -103,7 +104,9 @@ const RecentlySoldArtworksRail: React.FC<RecentlySoldArtworksRailProps> = ({
onPress={() => {
onPress?.(item, index)
}}
metaContainerStyles={{ height: 100 }}
metaContainerStyles={{
height: ARTWORK_RAIL_TEXT_CONTAINER_HEIGHT + 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Where does this + 10 come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think PixelRatio.getFontScale() cannot recognize when the Estimate goes into two lines, so we need to add the extra line height manually (10 is enough even tho the line height of the text is 20)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we achieve the same by making the overflow visible (e.g. overflow: visible)? I would prefer this solution instead of adding padding for this special case. Or we could make the height of the meta text container flexible which would be the most natural way for me to solve the issue (imagine the text needs to have 3 or 4 lines which could theoretically happen). 🤔

Copy link
Member

Choose a reason for hiding this comment

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

same thought here - You can also set the metaCardHeight to 110 if 100 is not enough

@artsyit artsyit dismissed stale reviews from olerichter00 and anandaroop via 52e9305 October 25, 2024 08:35
@@ -103,7 +104,9 @@ const RecentlySoldArtworksRail: React.FC<RecentlySoldArtworksRailProps> = ({
onPress={() => {
onPress?.(item, index)
}}
metaContainerStyles={{ height: 100 }}
metaContainerStyles={{
height: PixelRatio.getFontScale() * 100,
Copy link
Member

Choose a reason for hiding this comment

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

👏

MounirDhahri
MounirDhahri previously approved these changes Oct 25, 2024
@dariakoko dariakoko added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Oct 25, 2024
@artsy-peril artsy-peril bot merged commit 34f483a into main Oct 25, 2024
7 checks passed
@artsy-peril artsy-peril bot deleted the dariakoko/fix-SellWithArtsyRecentlySold-rail-layout branch October 25, 2024 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Jira Synced Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants