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.

Sign up for GitHub

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

Jump to bottom

fix: #213 #220

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: #213 #220

wants to merge 1 commit into from
+5 −0

Conversation

hyochan
Copy link

@hyochan hyochan commented Jan 20, 2024 •
edited

Description

One Line Summary

Temporary fixes on #213

Details

Motivation

REQUIRED -
Plugin fails to build on ios because of mission file.
Related: #213

Scope

RECOMMEND - OPTIONAL - What is intended to be effected. What is
Copy OneSignalNotificationServiceExtension-Info.plist file under ios dir.

Testing

Manual testing

REQUIRED - Explain what scenarios were tested and the environment.

Ensure that OneSignalNotificationServiceExtension-Info.plist is also in ios dir.

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have personally tested this on my device, or explained why that is not possible
  • I have tested this on the latest version of the plugin
  • I have tested this on both Android and iOS, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

@hyochan hyochan mentioned this pull request Jan 22, 2024
Closed
1 task
rgomezp
rgomezp reviewed Jan 23, 2024
View reviewed changes
onesignal/withOneSignalIos.ts
@@ -132,6 +132,11 @@ const withOneSignalNSE: ConfigPlugin<OneSignalPluginProps> = (config, props) =>
const extFile = NSE_EXT_FILES[i];
const targetFile = `${iosPath}/${NSE_TARGET_NAME}/${extFile}`;
await FileManager.copyFile(`${sourceDir}${extFile}`, targetFile);

if (extFile === 'OneSignalNotificationServiceExtension-Info.plist') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use NSE_TARGET_NAME here with the -Info.plist postfix

muhzain
muhzain approved these changes Jan 25, 2024
View reviewed changes
Copy link

@muhzain muhzain left a comment

Choose a reason for hiding this comment

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

LGTM. Working fine for us

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgomezp rgomezp rgomezp left review comments

@muhzain muhzain muhzain approved these changes

@ali-kamranch ali-kamranch ali-kamranch approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hyochan @rgomezp @muhzain @ali-kamranch