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

Feature/status 285 #11

Closed
wants to merge 1 commit into from

Conversation

danielgindi
Copy link
Contributor

@danielgindi danielgindi commented Aug 21, 2018

@mauron85
Copy link
Owner

Looking good, but please next time your PRs should be based on current master and not on the top of each other. Like this PR contains changes from #10. Should not be the case.

@mauron85
Copy link
Owner

The reason is obvious. With this PR you're forcing me to merge both PR 11 and 10. Not saying I'm not going to do that anyway, but I like to have a choice ;)

@danielgindi
Copy link
Contributor Author

There were conflicts so I kept it based on the other PR...
But of course I could rebase before you merge if that's what you want!

@mauron85
Copy link
Owner

Ok. no problem.

@danielgindi
Copy link
Contributor Author

I've rebased.
If merging with the other one, there will be a conflict in SyncAdapter, around these lines:

                if (isStatusOkay) {
                    builder.setContentText("Sync completed");
                } else {
                    builder.setContentText("Sync failed due server error");
                }

Those should be just wrapped in if (builder != null) {.

Btw I don't think we need the flavors for oreo and pre-oreo.
That SyncAdapter is mostly, like 99%, duplicated code.
It could be easily achieved with using either a one-line api version condition, or the support library which will just do nothing when referencing a notification channel.

@mauron85
Copy link
Owner

Btw I don't think we need the flavors for oreo and pre-oreo.

The reason is react-native by default creates new project with pre oreo sdk.

@danielgindi
Copy link
Contributor Author

Oh so the NotificationChannel does not exist at all right?
Btw I think they bumped the default sdk version in 0.57
https://github.com/react-native-community/react-native-releases/blob/0.57/CHANGELOG.md (targetSdkVersion is 26)

@mauron85
Copy link
Owner

If so, flavors can be removed. I was never actually fan, but it was just from necessity.

@danielgindi
Copy link
Contributor Author

Yes I got it... If there only were #defines in Java/Kotlin...

@mauron85
Copy link
Owner

Merged. Thank you!

@mauron85 mauron85 closed this Aug 23, 2018
@danielgindi danielgindi deleted the feature/status_285 branch August 24, 2018 10:45
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.

2 participants