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

feat: New architecture support #4574

Merged
merged 13 commits into from
Jan 9, 2025
Merged

Conversation

BogiKay
Copy link
Contributor

@BogiKay BogiKay commented Dec 6, 2024

Motivation

Main motivation is to make Paper great again by supporting new architecture, updating crucial deps and fixing as many as possible issues that occur when new-arch is enabled.

Main changes:

  1. dependencies updated
  2. new architecture enabled in example app
  3. migrated Menu component to functional one
  4. refreshed and slightly simplified Modal component
  5. fixed some issues linked below

Known issues:

  1. There's a problem with overflowing value of TextInput on iOS that should be addressed by [iOS] Fix paragraph styling of TextInput facebook/react-native#48060
  2. CVV TextInput is as wide as container is - I investigated that problem and it's caused by caching some of layout measurements. I created an issue [Android][NewArch] Weird layout styling if multiple instances of the same component with the same props facebook/react-native#48249 to take care of it and in the meantime I've debugging and looking for a fix.

Related issue

#4140
#4544
#4555
#4559
#4520
#4552

Test plan

We need to proceed with general regression testing using new and old architecture for all supported platforms.

Pay extra attention when testing Menu and Modal components.

@BogiKay BogiKay changed the title New architecture support feat: New architecture support Dec 6, 2024
@BogiKay BogiKay marked this pull request as draft December 6, 2024 14:51
@BogiKay BogiKay force-pushed the feat/new-arch branch 7 times, most recently from 7283aab to 07e8796 Compare December 8, 2024 14:18
@callstack-bot
Copy link

callstack-bot commented Dec 8, 2024

Hey @BogiKay, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@BogiKay BogiKay marked this pull request as ready for review December 9, 2024 14:32
@gedu
Copy link
Contributor

gedu commented Dec 9, 2024

Great!!! Is working awesome.
On iOS I saw these 2 things with TextInputs
Screenshot 2024-12-09 at 19 25 02

paper_textInput.mp4

Later I will try to look at Android and Web, If those issues are happening already we can open an issue for tracking

@BogiKay
Copy link
Contributor Author

BogiKay commented Dec 9, 2024

Great!!! Is working awesome. On iOS I saw these 2 things with TextInputs Screenshot 2024-12-09 at 19 25 02

paper_textInput.mp4
Later I will try to look at Android and Web, If those issues are happening already we can open an issue for tracking

Thanks for testing my changes 🙂

First issue is mentioned in "known issues" section in PR's description. I described what's the problem. I think we can come back to it later to figure out some kind of workaround after releasing current changes since this regression is not strictly related to the Paper.

When it comes to the second issue, there's an existing PR(#4497) that fixes it, so I decided to leave it as it is and review & test the mentioned PR once we merge this PR.

@gedu
Copy link
Contributor

gedu commented Dec 10, 2024

On android I couldn't run it at first because of this error:
Screenshot 2024-12-10 at 20 45 13

on the app.json file changing to this:

"runtimeVersion": "exposdk:52.0.0",

worked

@gedu
Copy link
Contributor

gedu commented Dec 10, 2024

I'm seeing an extra ripple effect and it is shown at the wrong place

extra_ripple_wrong_place.mp4

@gedu
Copy link
Contributor

gedu commented Dec 10, 2024

On ActivityIndicator custom size I'm seeing a white line

Screenshot 2024-12-10 at 20 52 29

@gedu
Copy link
Contributor

gedu commented Dec 10, 2024

Not sure which one is wrong or right but the X Chip is different on iOS vs Android

Screenshot 2024-12-10 at 20 57 32

@gedu
Copy link
Contributor

gedu commented Dec 10, 2024

On Android the CVV text input that should be small, they are filling the width and on focus they shrink

textinput_should_be_small.mp4

@gedu
Copy link
Contributor

gedu commented Dec 12, 2024

On web the Fab is all the way to the bottom, and it has some visual bugs

paper_web_fab.mp4

@gedu
Copy link
Contributor

gedu commented Dec 12, 2024

[Web] I think I didn't see it before, but custom corners on card shows a small white gap

Screenshot 2024-12-12 at 13 02 50

@gedu
Copy link
Contributor

gedu commented Dec 12, 2024

[Web] Seems the text is not center align on Chips
Screenshot 2024-12-12 at 13 07 02

@gedu
Copy link
Contributor

gedu commented Dec 12, 2024

[Web] On list.section the chip is cutoff

Screenshot 2024-12-12 at 13 09 29

@gedu
Copy link
Contributor

gedu commented Dec 12, 2024

[Web] Listitem I can't click checkboxes or switches

paper_web_listItem_noClick.mp4

@raajnadar
Copy link
Collaborator

Button color on loading state is also not correct please check that issue.

@BogiKay
Copy link
Contributor Author

BogiKay commented Dec 13, 2024

Little update:

iOS:

Android:

  • runtimeVersion error - I set based on sdk version. It's pity that it needs to be set manually. Once, fingerprint policy goes out of beta, we should think about it.
  • Ripple effect - Not able to reproduce. Working fine with latest changes.
  • Activity Indicator - a white line is present on both architectures and it's already existing regression, so it should be tackled as a separate work
  • Different look of chips - It's not an X chip, it contains a very long text that gets truncated and it looks different on different screens size-wise. Doesn't seem to be connected to new-arch. I'll tackle it separately
  • CVV TextInput is as wide as container is - I investigated that problem and it's caused by caching some of layout measurements. I created an issue [Android][NewArch] Weird layout styling if multiple instances of the same component with the same props facebook/react-native#48249 to take care of it and in the meantime I've debugging and looking for a fix. Update: implemented temporary workaround for this issue.

Web:

  • FAB - I've already fixed that in another PR fix(animated-fab): label styling (web) #4567
  • Card corners - It's not a regression related to new-arch. Most likely it has never been working well on web. The radius is applied to image only, not the card.
  • Chip text alignment - This issue is present in the last release of the library. It's not something caused by new arch. Will be taken care of individually.
  • List section chip - This issue is present in the last release of the library. It's not something caused by new arch. Will be taken care of individually.
  • List item checkboxes are not interactive - It's not intuitive, but it was implemented that way. We can adjust that behavior later on.
  • Button color when loading - Wasn't able to reproduce. @raajnadar can you provide reproducible example?

Help is more than welcomed, so if someone wants to work on something from the list above feel free to do that so and just leave a comment to let others be on the same page.

@raajnadar
Copy link
Collaborator

The issue of the disabled button disappeared after using the latest version of the Expo Go & expo router, if the issue reappears I will give more detailed repro

Comment on lines 34 to 36
yarn --cwd example --frozen-lockfile
yarn --cwd docs --frozen-lockfile
yarn --frozen-lockfile
Copy link
Member

Choose a reason for hiding this comment

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

--frozen-lockfile is for yarn 1, --immutable is for berry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Missed that due to backward compatibility. Updated

yarn install --cwd docs --frozen-lockfile
yarn install --frozen-lockfile
sudo corepack enable
yarn set version berry
Copy link
Member

@satya164 satya164 Dec 16, 2024

Choose a reason for hiding this comment

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

CI shouldn't call set version. It should be already set in the package.json. CI should use the same package manager everyone else would use when they run yarn directly in the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but currently circleCI image is used and they provide it with yarn1 only, so I had to do it that way to force usage of newer version. I'm thinking about updating the CI config and getting away from circleCI image that seem a bit deprecated

Copy link
Member

Choose a reason for hiding this comment

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

@BogiKay it will be automatically handled if setup like this https://github.com/callstack/react-native-paper/pull/4574/files#r1886705407

But also enabling corepack should already mean that it would use correct package manager from package.json and install it - so there shouldn't need to be an extra step for setting version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra step setting version explicitly removed. Thanks!

Comment on lines +77 to +86
const styles = StyleSheet.create({
stackWrapper: {
flex: 1,
...Platform.select({
web: {
overflow: 'scroll',
},
}),
},
});
Copy link
Member

Choose a reason for hiding this comment

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

Is React Navigation not working on Web? This shouldn't be needed

Copy link
Contributor Author

@BogiKay BogiKay Dec 16, 2024

Choose a reason for hiding this comment

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

Scrolling doesn't work without these changes. If I remember correctly it's caused by react-native-screens changes

Copy link
Member

Choose a reason for hiding this comment

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

It could've something to do with the styles in here that differ from default expo setup

https://github.com/callstack/react-native-paper/blob/main/example/public/index.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried the default ones from expo and it didn't help. I investigated that in the past and the breaking change is somewhere within components provided by navigation stack

@@ -331,7 +337,7 @@ const Button = (
buttonStyle,
style,
!isV3 && !disabled && { elevation },
] as ViewStyle
] as StyleProp<ViewStyle>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
] as StyleProp<ViewStyle>
] satisfies StyleProp<ViewStyle>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/components/Typography/AnimatedText.tsx Outdated Show resolved Hide resolved
.yarnrc.yml Show resolved Hide resolved
@BogiKay BogiKay force-pushed the feat/new-arch branch 2 times, most recently from fc35c95 to dc6c246 Compare December 16, 2024 16:06
@pmmmwh
Copy link

pmmmwh commented Dec 30, 2024

Found an issue with anchorPosition "bottom" with the new Menu implementation while importing this as a patch - on first trigger the anchoring is correct but on subsequent triggers it will revert to anchoring at the top.

@BogiKay
Copy link
Contributor Author

BogiKay commented Dec 31, 2024

Found an issue with anchorPosition "bottom" with the new Menu implementation while importing this as a patch - on first trigger the anchoring is correct but on subsequent triggers it will revert to anchoring at the top.

Thanks for catching it. Issue solved 👍

@BogiKay BogiKay requested a review from gedu December 31, 2024 11:24
@gedu
Copy link
Contributor

gedu commented Jan 7, 2025

Gave it another round, and LGTM

@JameDodgers
Copy link

JameDodgers commented Jan 7, 2025

It seems that changing disabled prop from false to true of AnimatedFAB component puts a weird shadow on the button on Android:

Captura de Tela 2025-01-07 às 17 37 22

@lukewalczak lukewalczak merged commit 77d3af7 into callstack:main Jan 9, 2025
3 checks passed
@lukewalczak lukewalczak added this to the 5.13.0 milestone Jan 9, 2025
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.

9 participants