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

Fix: Web rendering issues and welcome screen layout issue #2809

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

fpena
Copy link
Collaborator

@fpena fpena commented Oct 17, 2024

Please verify the following:

  • yarn test jest tests pass with new tests, if relevant
  • yarn lint eslint checks pass with new code, if relevant
  • yarn format:check prettier checks pass with new code, if relevant
  • README.md (or relevant documentation) has been updated with your changes
  • If this affects functionality there aren't tests for, I manually tested it, including by generating a new app locally if needed (see docs).

Describe your PR

Closes #2807

This PR solves 3 issues:

  1. Within the Screen component, KeyboardAvoidingView is crashing in web. It was replaced by a normal ScrollView.
  2. Within the useHeader hook and web, useLayoutEffect was creating a rendering loop. It is replaced by useEffect.
  3. Within the welcome screen and specifically in Spanish, the text clashes with the face icon. This was fixed.

Screenshots (if applicable)

Before After
https://github.com/user-attachments/assets/25330c6a-e93a-41be-87ea-b8d22779d0a2 https://github.com/user-attachments/assets/7a8a4cf3-bac4-4a6e-b3b8-e0741135bb4f

@fpena fpena changed the title Improvements Draft PR Oct 17, 2024
@fpena fpena changed the title Draft PR Fix: Web rendering issues and welcome screen layout issue Oct 17, 2024
@fpena fpena marked this pull request as ready for review October 17, 2024 20:37
Comment on lines 273 to 275
<ScrollView>
<ScreenWithoutScrolling {...props} />
) : (
<ScreenWithScrolling {...props} />
)}
</KeyboardAvoidingView>
</ScrollView>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this is a root-cause fix. I'd like to make sure we look into what's going on and why so we know that this is where we want to fix it.

Note that in the original stack trace, the root of the error actually occurs in KeyboardAwareScrollView.
Screenshot 2024-10-18 at 3 23 49 PM

Replacing KeyboardAwareScrollView also stops the issue from occurring:

function ScreenWithScrolling(props: ScreenProps) {
  ...
  useScrollToTop(ref)
  const MyComponent = Platform.OS === "web" ? ScrollView : KeyboardAwareScrollView

  return (
    <MyComponent
      bottomOffset={keyboardBottomOffset}
      {...{ keyboardShouldPersistTaps, scrollEnabled, ref }}

Also note that on the original issue @frankcalise linked this ticket where someone noted that adding import "setimmediate" also appears to fix the issue. I tried this out and it appears to work, as well (setimmediate is apparently brought in by fbjs which is a dependency of react-native-web.

I'm not opposed to an immediate fix to unblock things, but I think we definitely need to dig deeper into this to find and fix root cause.

I looked at the error in the dev console and it gives a clearer stack trace of the issue: that internal.js file is part of react-native-keyboard-controller. its use of setImmediate makes this not compatible with web.
Screenshot 2024-10-18 at 3 39 45 PM

I think the more appropriate fix is to swap out KeyboardAwareScrollView for a regular scrollview, since it wasn't designed for web.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, I think we should probably use a const to define a new component wrapping KeyboardAwareScrollView ? ScrollView like we did with the useEffect below, and export it from a file like it's a component. that way other components that may want to use this can get a version compatible with web, instead of everyone having to write a platform ternary.

boilerplate/app/utils/useHeader.tsx Show resolved Hide resolved
</KeyboardAvoidingView>
</ScrollView>
) : (
<KeyboardAvoidingView
Copy link
Contributor

@lindboe lindboe Oct 18, 2024

Choose a reason for hiding this comment

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

Why do we have KeyboardAvoidingView if we also have KeyboardAwareScrollView nested within it? If we need it for the non-scrolling version, I would think:

  1. We wouldn't want to wrap KeyboardAvoidingView around both cases, only non-scrolling
  2. We wouldn't want to mix RN's KeyboardAvoidingView with RNKC's KeyboardAwareScrollView. It looks like RNKC has its own KeyboardAvoidingView.

@@ -57,7 +57,7 @@ export const WelcomeScreen: FC<WelcomeScreenProps> = observer(
tx="welcomeScreen:readyForLaunch"
preset="heading"
/>
<Text tx="welcomeScreen:exciting" preset="subheading" />
<Text tx="welcomeScreen:exciting" preset="subheading" style={$subheading} />
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes don't seem to render right for me on web:
Screenshot 2024-10-18 at 3 51 56 PM

* therefore we want to use ScrollView just for web
*/}
{Platform.OS === "web" ? (
<ScrollView>
<ScreenWithoutScrolling {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Also meant to ask: why does web only get ScreeenWithoutScrolling? Shouldn't it have both options?

I think that's why this appeared to fix things, it removes KeyboardAwareScrollView from the document.

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.

Ignite X web crashes
2 participants