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

Make osx example utilise the common example app #3055

Merged
merged 85 commits into from
Oct 1, 2024

Conversation

latekvo
Copy link
Contributor

@latekvo latekvo commented Aug 21, 2024

Description

This PR makes the MacOS example utilise the common example app.
This change will allow for more in-depth testing of MacOS functionalities.

For now, it's still work in progress

Test plan

Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

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

I hope it won't have much conflicts with #2919 😅

MacOSExample/package.json Outdated Show resolved Hide resolved
@latekvo
Copy link
Contributor Author

latekvo commented Aug 22, 2024

I hope it won't have much conflicts with #2919 😅

I hope there won't be any, App.tsx should remain minimally changed with just a couple of styles differing, while the rest of the changes should be out of scope of #2919

Regardless, this PR is still WIP, and only works on my end due to some modifications to App.tsx structure, as I'm still working out some rendering inconsistencies, which are hopefully caused by default styling differences.

@latekvo
Copy link
Contributor Author

latekvo commented Aug 23, 2024

As of now, the common examples are working on MacOS, and I am still working on completely removing the MacOSExample directory, but that may prove challenging as currently we're relying on the babel.config.js to make the entire app work on MacOS, which unfortunately would prevent other platforms from using the examples if we were to naively marge both of these folders in the state that they are.

image

(cherry picked from commit 72fd9dd)
@latekvo
Copy link
Contributor Author

latekvo commented Sep 25, 2024

Screen.Recording.2024-09-24.at.18.43.34.mov

Fixed header being squashed in these commits: e3c098d, 6a6f885, 38f861d (cherry-picked from this branch)

@latekvo
Copy link
Contributor Author

latekvo commented Sep 26, 2024

Removed chevrons in 61455cf, couldn't find a way to properly load their custom font.
Tried:

  • the formal method of adding fonts found here
  • modifying react-native-asset tool to work with macos, the tool worked but the fonts still weren't present
  • started working on dynamic font loader based on this 2012 article and this StackOverflow answer but decided it's not worth spending too much time on this feature for now.

All other known bugs have been resolved by now as far as i know.

Please let me know if there're any other changes you'd like to see, or if it's ok to merge this PR.

MacOSExample/babel.config.js Show resolved Hide resolved
MacOSExample/macos/Podfile Show resolved Hide resolved
MacOSExample/package.json Outdated Show resolved Hide resolved
example/App.tsx Outdated Show resolved Hide resolved
example/src/ListWithHeader/Header.tsx Outdated Show resolved Hide resolved
Copy link
Member

@j-piasecki j-piasecki 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, few things I noticed:

  • Draggable example using the old API doesn't work, I don't recall whether it did at some point (cc @m-bert), but it might be worth looking into
  • Pressable doesn't work, which is not ideal

Bot those can be addressed in follow-ups, as this PR has been going on for quite some time

Comment on lines +19 to +20
export const HEADER_HEIGHT =
Platform.OS === 'web' ? 64 : Platform.OS === 'macos' ? 128 : 192;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the header the same as on web? Images seem to always be stretched, it may make sense to set explicit sizes for them instead of relying on the platform to properly contain them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll open a separate PR for it to avoid cluttering this one any further.

@m-bert
Copy link
Contributor

m-bert commented Oct 1, 2024

Okay, I've looked into Draggable example and it seems to be the problem with NativeViewGestureHandler (something else than I expected). I'll dive into it later.

Also what I've noticed is that sometimes app gets stuck at bundling overlay:

image

Copy link
Member

@j-piasecki j-piasecki left a comment

Choose a reason for hiding this comment

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

To sum up, I think we can merge this as-is currently (but please, use Hermes) and then create follow-up issues for:

  • header looking weird on macOS
  • Draggable example not working (for other reasons than expected)
  • Pressables not working on macOS
  • app being stuck on loading bundle screen

and keep working on these

(use Maintainer issue label to stop bot from bothering you).

@latekvo latekvo requested a review from m-bert October 1, 2024 15:35
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

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

Remember to address things mentioned by @j-piasecki in follow-ups 😅

@latekvo latekvo merged commit 2183711 into main Oct 1, 2024
3 checks passed
@latekvo latekvo deleted the @latekvo/support-osx-in-primary-example branch October 1, 2024 16:17
latekvo added a commit that referenced this pull request Oct 4, 2024
## Description

There are 2 `Pressable` entries, likely as a result of merge conflicts
which occurred between these two PRs:

[Update
examples](https://github.com/software-mansion/react-native-gesture-handler/pull/2919/files)
and [Make osx example utilise the common example
app](#3055)
@m-bert m-bert restored the @latekvo/support-osx-in-primary-example branch October 24, 2024 12:30
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.

3 participants