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

Added support of pull down to refresh feature for Android platform. #2787

Conversation

muskan273
Copy link

No description provided.

Copy link

@Kasea Kasea left a comment

Choose a reason for hiding this comment

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

First, I love how "fast" react-native libraries/developers are when it comes to "bugs" that have existed in their libraries for years that are a huge deal breaker 😄.

Getting to the PR though; There are a couple of problems which are a "deal breaker" (imo).

When dealing with a public project or any project for that matter, ALWAYS conform to the code bases code style/practices.
This includes how they leave comments, indentation, ordering of methods (although it might be random, see next point), method/property naming and everything else inbetween.
You've mostly done this, although there are places where you've changed indentation (to the correct one technically, but see next point) and added spaces.

When dealing with a public project, always keep the PRs as small as possible. I can see you've "fixed" a lot of formatting "errors", but this just adds to more code to review. Keep it as simple as possible. If this was at a company I would be happy to see some code cleanup, but these developers are not paid and want to do the least possible 😓 . You've also moved around a couple of methods and in some cases duplicated them. Don't do unnecessary work!

@Kasea
Copy link

Kasea commented Jan 31, 2023

NOTE: if you have a modal with overflow, scroll down on said modal, release and then try to scroll up, it'll refresh instead of scrolling up.

This is working as intended on iOS

@snaptravel-eng
Copy link

snaptravel-eng commented Feb 28, 2023

Hi @Kasea @muskan273, what do we need to do to get this merged in? Is the only issue the code style/formatting or is the modal with overflow a dealbreaker?

@stanimirsellercloud
Copy link

stanimirsellercloud commented Feb 28, 2023

@snaptravel-eng I think this also fixes pull to refresh on Android. It seems to be working on iOS only.

@snaptravel-eng
Copy link

Thanks. Yeah I understand the intention. How do we get this merged instead of stuck as a PR?

@stanimirsellercloud
Copy link

cc @Titozzz @Saadnajmi

@rafaelmonroy
Copy link

does anyone know when this is getting merged? 👀 @muskan273 @Kasea @Titozzz @Saadnajmi

@muskan273
Copy link
Author

muskan273 commented Mar 18, 2023

@Kasea @jamonholmgren @Titozzz Can you please review the latest commit? I have made changes as suggested above. Let me know if I need to make any changes.

@snaptravel-eng
Copy link

Thanks for updating @muskan273 !

How do we get this merged instead of stuck as a PR? @Kasea @jamonholmgren @Titozzz

@muskan273
Copy link
Author

NOTE: if you have a modal with overflow, scroll down on said modal, release and then try to scroll up, it'll refresh instead of scrolling up.

This is working as intended on iOS

@muskan273 muskan273 closed this Apr 11, 2023
@muskan273 muskan273 reopened this Apr 11, 2023
@muskan273
Copy link
Author

muskan273 commented Apr 11, 2023

NOTE: if you have a modal with overflow, scroll down on said modal, release and then try to scroll up, it'll refresh instead of scrolling up.

This is working as intended on iOS

I faced the same issue while implementing that feature. I have mentioned the solution in the below blog:
https://dev.to/muskanparashar/react-native-webview-make-android-pulltorefresh-work-without-any-glitches-467

@peterlazar1993
Copy link
Contributor

@muskan273 Congrats on your first PR!
I'd love to test this, could you please rebase master.

@Muneeb926595
Copy link

Muneeb926595 commented Apr 20, 2023

is there anyone who can rebase master please i need this PR urgently.

@snaptravel-eng
Copy link

Is there any blocker for updating and merging this? The open bounty still applies: #2678
Thanks

@StephenCavender
Copy link

this pr doesn't work for loading web pages that have nested scroll views. The page i'm trying to load has a scrollable content block between a sticky header and footer. I can't scroll up within the scrollable content, it just triggers the refresh.

Repro is to replace the example for Scrolling with this:

import React, { Component } from 'react';
import { Button, Text, View } from 'react-native';

import WebView from 'react-native-webview';

const HTML = `
<!DOCTYPE html>\n
<html>
  <head>
    <title>Hello World</title>
    <meta http-equiv="content-type" content="text/html; charset=utf-8">
    <meta name="viewport" content="width=320, user-scalable=no">
    <style type="text/css">
    #flexbox_container {
      display: flex;
      flex-direction: column;
      box-shadow: 0 4px 5px -2px #999;
      height: 90%;
    }
    
    .flex_item {
      background: #eee;
      margin: 4px;
      padding: 10px;
    }
    
    .middle {
      flex-grow: 1;
      overflow: auto;
    }
    
    html,
    body {
      height: 100%;
    }
    </style>
  </head>
  <body>
  <div id="flexbox_container">
  <div class="flex_item">
    Item</div>
  <div class="flex_item middle">
    <p>1</p>
    <p>2</p>
    <p>3</p>
    <p>4</p>
    <p>5</p>
    <p>6</p>
    <p>7</p>
    <p>8</p>
    <p>9</p>
    <p>10</p>
    <p>11</p>
    <p>12</p>
    <p>13</p>
  </div>
  <div class="flex_item">Item</div>
</div>
  </body>
</html>
`;

type Props = {};
type State = {
  scrollEnabled: boolean;
  lastScrollEvent: string;
};

export default class Scrolling extends Component<Props, State> {
  state = {
    scrollEnabled: true,
    lastScrollEvent: '',
  };

  render() {
    return (
      <View>
        <View style={{ height: 240 }}>
          <WebView
            source={{ html: HTML }}
            automaticallyAdjustContentInsets={false}
            onScroll={this._onScroll}
            scrollEnabled={this.state.scrollEnabled}
          />
        </View>
        <Button
          title={
            this.state.scrollEnabled ? 'Scroll enabled' : 'Scroll disabled'
          }
          onPress={() =>
            this.setState({ scrollEnabled: !this.state.scrollEnabled })
          }
        />
        <Text>Last scroll event:</Text>
        <Text>{this.state.lastScrollEvent}</Text>
      </View>
    );
  }

  _onScroll = (event) => {
    this.setState({ lastScrollEvent: JSON.stringify(event.nativeEvent) });
  };
}


Recording of behavior:
https://user-images.githubusercontent.com/6619472/235944464-2d826e6e-4ef2-40ff-853b-a682b6f54169.mov

@github-actions
Copy link

github-actions bot commented Jul 3, 2023

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Jul 3, 2023
@github-actions github-actions bot closed this Jul 10, 2023
@muskan273
Copy link
Author

muskan273 commented Aug 22, 2023

Hi, I wanted to bring your attention back to a closed pull request that I believe requires further consideration. I have identified a solution for the issue discussed in this comment: #2787 (comment).
You can review the proposed solution in the attached file provided below:

WebviewRefresh.js.zip

Looking forward to your feedback and the possibility of reopening and resolving this pull request.
@Kasea @StephenCavender @snaptravel-eng @stanimirsellercloud @rafaelmonroy @peterlazar1993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants