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

fix(ONYX-1380): Activity Item screen spacing #10988

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(ONYX-1380): Activity Item screen spacing #10988

wants to merge 3 commits into from
+22 −17

Conversation

olerichter00
Copy link
Contributor

@olerichter00 olerichter00 commented Oct 22, 2024 •
edited
Loading

Resolves https://artsyproduct.atlassian.net/browse/ONYX-1380

Description

The spacing below the artwork list on the Activity Item screen is a bit off. This might be related to some adjustments made to the partner offer notification screen.

I've tested this fix with all notification types, and it seems to work. But I'm not completely sure about the Partner offer's note and commercial actions. 🤔

Before After
Simulator Screenshot - iPhone 15 Plus - 2024-10-28 at 12 58 30 Simulator Screenshot - iPhone 15 Plus - 2024-10-28 at 12 57 09
--- ---
Simulator Screenshot - iPhone 15 Plus - 2024-10-28 at 12 59 14 Simulator Screenshot - iPhone 15 Plus - 2024-10-28 at 13 00 22
--- ---
Simulator Screenshot - iPhone 15 Plus - 2024-10-28 at 12 58 16 Simulator Screenshot - iPhone 15 Plus - 2024-10-28 at 12 57 40

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

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.

@olerichter00 olerichter00 self-assigned this Oct 22, 2024
@olerichter00 olerichter00 marked this pull request as draft October 22, 2024 08:43
@olerichter00 olerichter00 force-pushed the olerichter00/activity-item-screen-spacing branch 2 times, most recently from 0eb932b to 90616fc Compare October 28, 2024 11:50
@olerichter00 olerichter00 changed the title fix: Activity Item screen spacing fix(ONYX-1380): Activity Item screen spacing Oct 28, 2024
@olerichter00 olerichter00 marked this pull request as ready for review October 28, 2024 12:05
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Oct 28, 2024 •
edited
Loading

Warnings
⚠️

❌ An error occurred while validating your changelog, please make sure you provided a valid changelog.

Generated by 🚫 dangerJS against 3fca2a8

olerichter00
olerichter00 commented Oct 28, 2024
View reviewed changes
src/app/Components/ArtworkGrids/MasonryInfiniteScrollArtworkGrid.tsx
@@ -127,7 +127,6 @@ export const MasonryInfiniteScrollArtworkGrid: React.FC<MasonryInfiniteScrollArt
contentContainerStyle={{
// No paddings are needed for single column grids
paddingHorizontal: getAdjustedNumColumns() === 1 ? 0 : space(2),
paddingBottom: artworks.length === 1 ? 0 : space(6),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to display the extra spacing if there are no more artworks to load because on the Activity screen we display content below the list.

olerichter00
olerichter00 commented Oct 28, 2024
View reviewed changes
src/app/Scenes/Activity/components/NotificationArtworkList.tsx
@@ -48,14 +48,15 @@ export const NotificationArtworkList: FC<NotificationArtworkListProps> = (props)
onScroll={scrollHandler}
partnerOffer={partnerOffer}
priceOfferMessage={priceOfferMessage}
style={{ paddingBottom: 120 }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This bottom padding seems to be unnecessary as well...

@olerichter00 olerichter00 force-pushed the olerichter00/activity-item-screen-spacing branch from 28c8ffd to 6dc1a55 Compare October 28, 2024 12:11
@olerichter00 olerichter00 force-pushed the olerichter00/activity-item-screen-spacing branch from 6dc1a55 to 3fca2a8 Compare October 28, 2024 12:13
olerichter00
olerichter00 commented Oct 28, 2024
View reviewed changes
src/app/Components/ArtworkGrids/MasonryInfiniteScrollArtworkGrid.tsx
@@ -140,7 +139,7 @@ export const MasonryInfiniteScrollArtworkGrid: React.FC<MasonryInfiniteScrollArt
refreshControl={refreshControl}
renderItem={renderItem}
ListFooterComponent={
<AnimatedMasonryListFooter shouldDisplaySpinner={shouldDisplaySpinner} />
hasMore ? <AnimatedMasonryListFooter shouldDisplaySpinner={shouldDisplaySpinner} /> : null
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to display the spinner component when there are no more artworks to be loaded because it creates extra spacing when we display content below the artwork list (e.g., on the Activity screen).

MrSltun
MrSltun reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@MrSltun MrSltun left a comment

Choose a reason for hiding this comment

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

Looks good! 🌟
It would be good to also add screenshots for Android as well :)

@olerichter00
Copy link
Contributor Author

olerichter00 commented Oct 28, 2024 •
edited
Loading

It would be good to also add screenshots for Android as well :)

I'm struggling to get the Android simulator to work... Could you please help test the changes on Android? 🙏 I can help create test notifications from these instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrSltun MrSltun MrSltun left review comments

@MounirDhahri MounirDhahri MounirDhahri left review comments

@narikazu narikazu Awaiting requested review from narikazu

@gkartalis gkartalis Awaiting requested review from gkartalis

Assignees

@olerichter00 olerichter00

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@olerichter00 @ArtsyOpenSource @MounirDhahri @MrSltun