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 listener in hooks #70

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

Conversation

hoanglam10499
Copy link

@hoanglam10499 hoanglam10499 commented Mar 13, 2020

Solved Issues #69.
Please merge

@hoanglam10499
Copy link
Author

hoanglam10499 commented May 19, 2020

Solved Issues #69.
Please merge

@Cmion
Copy link

Cmion commented May 21, 2020

I think using a callback for the listenToOrientationChange would be a better choice.
And you seem to be re-writing most of the file.
I guess it's your formatter.

so consider something like.

`

if (typeof callback === 'function' ){
  const orientation = screenWidth < screenHeight ? 'portrait' : 'landscape';
  callback(orientation);

};`

Lesser code and solves the problem for functional and class based components.

Try refactoring your code, and make changes only to the lines that concerns your PR.

@hoanglam10499
Copy link
Author

@Cmion
I didn't change all the files, it was a change of the format code.
You can see the "File Changed".

Screen Shot 2020-05-22 at 08 47 26

@Cmion
Copy link

Cmion commented May 22, 2020

Fix listener in hooks by hoanglam10499 · Pull Request #70 · marudy_react-native-responsive-screen - Google Chrome 5_22_2020 18_23_46
Fix listener in hooks by hoanglam10499 · Pull Request #70 · marudy_react-native-responsive-screen - Google Chrome 5_22_2020 18_24_02
Fix listener in hooks by hoanglam10499 · Pull Request #70 · marudy_react-native-responsive-screen - Google Chrome 5_22_2020 18_24_10

I'm not a maintainer, contributor or anything.
But It seems like your recent commit made those changes.

@Cmion
Copy link

Cmion commented May 22, 2020

I also looks like the library needs some maintenance too

@hoanglam10499
Copy link
Author

@Cmion It was a change of the format code ...

@gregfenton
Copy link

gregfenton commented Aug 7, 2020

Some quick, hopefully constructive, feedback on this PR:

  • to keep the PR clean, get rid of the commented out code. We have version control 😃
  • as @Cmion suggests in his post, consider using === for the comparison operator
  • I'd like to see this more explicit in terms of what parameter it is getting/using. Maybe have 2 parameters:
    const listenOrientationChange = ({classComponentThis = null, setStateHook = null}) => {
        // . . .
        if (classComponentThis) {
          classComponentThis.setState(...);
        } else if (setStateHook) {
          setStateHook(...);
        }
      }
    

Thank you for your efforts on this! Really, really appreciated.

gregfenton added a commit to gregfenton/react-native-responsive-screen that referenced this pull request Aug 7, 2020
Encapsulate the changes inspired by marudy#70
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