Skip to content

Comments

feat: add follows to favorites tab#11728

Merged
artsy-peril[bot] merged 5 commits intomainfrom
feat/add-follows-to-favorites
Mar 19, 2025
Merged

feat: add follows to favorites tab#11728
artsy-peril[bot] merged 5 commits intomainfrom
feat/add-follows-to-favorites

Conversation

@MounirDhahri
Copy link
Member

@MounirDhahri MounirDhahri commented Mar 19, 2025

This PR resolves ONYX-1602

Description

This PR adds the follows tab to the favorites bottom tab.

Android iOS
Screen.Recording.2025-03-19.at.16.01.31.mov
Screen.Recording.2025-03-19.at.16.00.18.mov

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

  • add follows to favorites tab - mounir

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.

@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Mar 19, 2025

This PR contains the following changes:

  • Cross-platform user-facing changes (add follows to favorites tab - mounir - MounirDhahri)

Generated by 🚫 dangerJS against 84d5d83

olerichter00
olerichter00 previously approved these changes Mar 19, 2025
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.

Looks great! 🌟

}}
keyExtractor={(item, index) => item.artist?.id || index.toString()}
contentContainerStyle={{ paddingVertical: space(1) }}
onEndReachedThreshold={0.2}
Copy link
Contributor

Choose a reason for hiding this comment

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

question: what's the reason to not use the default here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept this to be the same as use it for the current FavortesArtists - there was no comment about it so I assumed it's probably no accidental

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot 2025-03-19 at 16 52 23

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, 0.2 seems to be our default. I've created a ticket to revisit and adjust the threshold for the artwork grid where pages are loaded quite late. I think we can improve the "scrolling through the artwork grid" experience by loading pages earlier.

contentContainerStyle={{ paddingVertical: space(1) }}
onEndReachedThreshold={0.2}
refreshControl={RefreshControl}
style={{ paddingHorizontal: 0 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately <Tabs.FlashList /> injects its own paddingHorizontal from palette and this is to override it.

key={value}
onPress={() => {
setfollowOption(value)
// Dismiss after a short delay to make sure the user can verify their choice
Copy link
Contributor

Choose a reason for hiding this comment

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

question: This code automatically closes the bottom sheet when the user clicks on the radio button, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve experimented with it, and I think we could remove or at least reduce the timeout to make it snappier.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will decrease it, and if it doesn't look good I can remove it

Copy link
Contributor

@dariakoko dariakoko left a comment

Choose a reason for hiding this comment

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

Looks great

onEndReached={() => {
loadNext(PAGE_SIZE)
}}
keyExtractor={(item, index) => item.id || index.toString()}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: can't we just use item.id here? I would expect it to always be present

Copy link
Member Author

Choose a reason for hiding this comment

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

id can be null from MP - it's never null though in practice

@artsy-peril artsy-peril bot merged commit 517ae86 into main Mar 19, 2025
7 checks passed
@artsy-peril artsy-peril bot deleted the feat/add-follows-to-favorites branch March 19, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants