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

chore: Activity screen refactoring & code decoupling #11032

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

olerichter00
Copy link
Contributor

@olerichter00 olerichter00 commented Oct 29, 2024

Follows up on #10988

Description

This is an attempt to refactor and decouple the Activity screen code to avoid bugs like the one fixed in #10988. I noticed that the artwork list component, which is used for many activity types, contains some type-specific elements and optimizations and tried to move them out of the component.

Before After (iOS) After (Android)
Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 33 48 Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 34 25 Screenshot_20241104_130202
--- --- ---
Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 35 05 Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 35 11 Screenshot_20241104_130334
--- --- ---
Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 35 48 Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 35 28 Screenshot_20241104_130409
--- --- ---
Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 36 07 Simulator Screenshot - iPhone 15 Plus - 2024-11-04 at 12 36 26 Screenshot_20241104_130527

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 29, 2024
@olerichter00 olerichter00 changed the title chore: Activity screen refactoring & decoupling chore: Activity screen refactoring & code decoupling Oct 29, 2024
@olerichter00 olerichter00 force-pushed the olerichter00/activity-screen-refactoring branch from 148be91 to 6c6e0f7 Compare November 1, 2024 15:09
@olerichter00 olerichter00 marked this pull request as ready for review November 1, 2024 15:10
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Nov 1, 2024

This PR contains the following changes:

Generated by 🚫 dangerJS against 443729f

@olerichter00 olerichter00 force-pushed the olerichter00/activity-screen-refactoring branch from 6c6e0f7 to b550b1c Compare November 4, 2024 08:48
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.

This looks good!

But I can see that there are changes in the rendered elements code, maybe we should test it and take screenshots before and after to make sure that this changes doesn't introduce a visual bug?

@olerichter00
Copy link
Contributor Author

But I can see that there are changes in the rendered elements code, maybe we should test it and take screenshots before and after to make sure that this changes doesn't introduce a visual bug?

That's a very good idea 👍 I've tested the changes with all notification types but haven't taken screenshots. I'll add them to the PR.

@olerichter00 olerichter00 merged commit 096ae6d into main Nov 4, 2024
7 checks passed
@olerichter00 olerichter00 deleted the olerichter00/activity-screen-refactoring branch November 4, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants