zoontek / react-native-permissions

An unified permissions API for React Native on iOS, Android and Windows.
MIT License
4.1k stars 836 forks source link

Updates README's Setup instructions' grammar & clarity #865

Closed Maker-Mark closed 7 months ago

Maker-Mark commented 7 months ago

Summary

I noticed the README had a spelling mistake in step 1 & the setup's clairity could be improved, so I took a stab at it.

Test Plan

N/A

Checklist

zoontek commented 7 months ago

@Maker-Mark Hi, and thanks! I agree with some parts, but strongly disagree with some others.

  1. The added indentation is useless, just makes it less readable on mobiles:
Screenshot 2024-04-12 at 10 36 23
  1. The instructions for react-native < 0.72 are incorrect.
  2. Removing the git diff format is not a good idea (having the green / red highlighting to quickly understand what to remove / add is important)
  3. You broke the diagrams in multiple places, as you fixed it for the editor, but not for the viewer (it was not aligned on purpose - see how it's currently correctly aligned):
Screenshot 2024-04-12 at 10 44 38

Please fix all of this first.

Maker-Mark commented 7 months ago

@zoontek Sure thing, I'll fix the indentation (1) and formatting (4). Regarding RN < 0.72 instructions, what's wrong with them/what should it be? The instructions before were not clear. Is it that you keep the - (removes) from the diff above and add the require_relative to the top of the file or what? If I was a RN 0.72 user that's what I'd interpret it as, so clarity on that would be helpful, maybe i'm missing something.

Regarding the diff (3). Why is there a diff there? Users setting this up for the first time wont have the old code to delete(correct me if I'm wrong), so it's a bit confusing as a diff. Maybe a separate note for that being removed if you're on an earlier version makes more sense?

Thanks for the quick responses and making this :-) here to make it better <3

zoontek commented 7 months ago

@Maker-Mark Red lines (git suppressions) should be removed, green lines (git additions) must be added. This means that on react-native < 0.72, you only need to add this line:

require_relative '../node_modules/react-native-permissions/scripts/setup'

Under the two existing ones:

require_relative '../node_modules/react-native/scripts/react_native_pods'
require_relative '../node_modules/@react-native-community/cli-platform-ios/native_modules'

Check the default Podfile for those versions.

Same for >= 0.72 (their default Podfile). The default require Pod::Executable.execute_command… must be replaced with a generic function to require both react-native/scripts/react_native_pods.rb and react-native-permissions/scripts/setup.rb.

Regarding the diff (3). Why is there a diff there? Users setting this up for the first time wont have the old code to delete(correct me if I'm wrong), so it's a bit confusing as a diff.

I explained it, hope it's clear. I disagree that removing the diff would made this clearer, as this is very common when you are doing code review (to see the deleted / added code), people are used to it. But adding some comments over each block might help.

Maker-Mark commented 7 months ago

@Maker-Mark Red lines (git suppressions) should be removed, green lines (git additions) must be added. This means that on react-native < 0.72, you only need to add this line:

require_relative '../node_modules/react-native-permissions/scripts/setup'

Under the two existing ones:

require_relative '../node_modules/react-native/scripts/react_native_pods'
require_relative '../node_modules/@react-native-community/cli-platform-ios/native_modules'

Check the default Podfile for those versions.

Same for >= 0.72 (their default Podfile). The default require Pod::Executable.execute_command… must be replaced with a generic function to require both react-native/scripts/react_native_pods.rb and react-native-permissions/scripts/setup.rb.

Regarding the diff (3). Why is there a diff there? Users setting this up for the first time wont have the old code to delete(correct me if I'm wrong), so it's a bit confusing as a diff.

I explained it, hope it's clear. I disagree that removing the diff would made this clearer, as this is very common when you are doing code review (to see the deleted / added code), people are used to it. But adding some comments over each block might help.

Ahh okay--thanks for the explanations, I really appreciate it. Always learning. Got it, I'll add it back with the diff and add a note. Plus, change the 0.72 instructions back.

Maker-Mark commented 7 months ago

@zoontek Done! Let me know how it's looking :)

zoontek commented 7 months ago

Made some additional changes to fix the typos: https://github.com/zoontek/react-native-permissions/pull/868/commits/4c35f4f967d34ca75adf32cf7cc6371439e0766e