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

docs: remove version_number example in ios-notification-images docs #8184

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

petetnt
Copy link
Contributor

@petetnt petetnt commented Dec 11, 2024

Description

This PR fixes an issue in the ios-notification-images.md doc, the doc tells to update VERSION_NUMBER but there's no version set in the example. In the gif below the image you can see the VERSION_NUMBER being set if you are fast enough.

The text:

Make sure to change the version number VERSION_NUMBER with the currently installed version (check your Podfile.lock)

Screenshot from the gif:

image

As per discussions, we removed the VERSION_NUMBER example from the docs

Release Summary

docs: removed VERSION_NUMBER example in the ios-notification-image.md docs

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

Follow the guide, add the version number and run pod install

Copy link

vercel bot commented Dec 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 2:04pm

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Hey there!

I think this should go the other way - if the Podfile has versions for that Pod set in some other way (and it does, for the main parts of react-native-firebase) - then you should not need a version number here as it should already be resolved to the one that the rest of the app is using, right?

So what should change I think is that the docs shouldn't tell you to update any VERSION_NUMBER anywhere for the iOS notification images.

This line would go away https://github.com/invertase/react-native-firebase/pull/8184/files#diff-05f38d5729372a6ce74811f5d33eb72c9a6b290203486ad013c80d1c3bc88f26R41

What needs to happen is to confirm that your build works with no version number (it should?) and then we can just get rid of that step entirely which should simplify things for people and give them one less thing to do in the Podfile on an ongoing basis - trying to keep up with those version number

@petetnt petetnt changed the title docs: fix version number example in ios-notification-images docs docs: remove version number example in ios-notification-images docs Dec 18, 2024
@petetnt petetnt changed the title docs: remove version number example in ios-notification-images docs docs: remove version_number example in ios-notification-images docs Dec 18, 2024
@petetnt
Copy link
Contributor Author

petetnt commented Dec 18, 2024

Hi @mikehardy!

Makes sense, I removed the VERSION_NUMBER and the related line about it and rebased against master in a061678

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks right, but just to make extra sure @petetnt

What needs to happen is to confirm that your build works with no version number (it should?)

Can you confirm that with no version number at all, this still works for you and builds the notification extension correctly? It should but confirmation is important :-)

Thanks!

@mikehardy mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. Workflow: Pending Merge Waiting on CI or similar type: documentation and removed Needs Attention labels Dec 18, 2024
@petetnt
Copy link
Contributor Author

petetnt commented Dec 18, 2024

@mikehardy Yeah sorry, should have clarified that I removed the version_number locally, ran pod install again and rebuilt the app and the extension still worked as expected :)

@mikehardy
Copy link
Collaborator

Fantastic, really appreciate it

@mikehardy mikehardy merged commit 9d2817b into invertase:main Dec 18, 2024
9 of 11 checks passed
@mikehardy mikehardy removed Workflow: Waiting for User Response Blocked waiting for user response. Workflow: Pending Merge Waiting on CI or similar labels Dec 18, 2024
@petetnt petetnt deleted the patch-1 branch December 18, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants