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

Introduce scrollEnabled property, refactor a way of setting the WebView settings, and apply minor fixes to the native codebase #190

Merged
merged 15 commits into from
Apr 24, 2024

Conversation

pklatka
Copy link
Collaborator

@pklatka pklatka commented Apr 19, 2024

Summary

This PR:

  1. Implements scrollEnabled property
  2. Modifies the approach of applying the settings in the WebView
  3. Applies minor fixes to the native codebase

Test plan

Tested if dynamically changing the scrollEnabled works properly in the demo app:

  1. Tested if the prop is updated on the native side when on the same webpage (e.g. setTimeout which changes the value)
  2. Tested if the changes are applied when going back to the previous screen

@pklatka pklatka changed the title Introduce scrollEnabled property Introduce scrollEnabled property, refactor a way of setting the WebView settings, apply minor fixes to native codebase Apr 22, 2024
@pklatka pklatka changed the title Introduce scrollEnabled property, refactor a way of setting the WebView settings, apply minor fixes to native codebase Introduce scrollEnabled property, refactor a way of setting the WebView settings, and apply minor fixes to the native codebase Apr 22, 2024
@pklatka pklatka marked this pull request as ready for review April 22, 2024 19:51
@pfeiffer
Copy link
Contributor

Had a look at at least the iOS code and I'm wondering if a slightly different approach could be taken:

@objc var scrollEnabled {
  didSet {
    webView?.isScrollEnabled = scrollEnabled
  }
}

...

func visitableDidAppear(..) // or visitableDidRender? {
  webView?.isScrollEnabled = scrollEnabled
}

Could even be refactored to a method, eg configureWebView if we have a lot of options that need to be set on the WebView, eg.:

@objc var scrollEnabled {
  didSet {
     configureWebView()
  }
}

@objc var contentInset {
  didSet {
     configureWebView()
  }
}
...

func visitableDidAppear(..) // or visitableDidRender? {
  configureWebView()
}

func configureWebView() {
   guard webView != nil else { return }

   webView.isScrollEnabled = scrollEnabled
   webView.contentInset = contentInset
   ...
}

@pklatka pklatka merged commit 1de29dd into main Apr 24, 2024
1 check passed
@pklatka pklatka deleted the scroll-enabled branch April 24, 2024 12:39
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.

2 participants