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

Issue 42 #229

Closed
wants to merge 8 commits into from
Closed

Issue 42 #229

wants to merge 8 commits into from

Conversation

0xapurv
Copy link
Contributor

@0xapurv 0xapurv commented May 7, 2021

Related Issue

Proposed Changes

  • Changed advance_pdf_viewer to native_pdf_view to avoid pubspec conflicts
  • added push notification serrvice

Additional Information

  • Any additional information about the change

Checklist

  • Tested
  • No Conflicts
  • Change In Code

ScreenVideo

relic_bazar_push_notif.mp4

@github-actions github-actions bot added the GSSOC21 Issues created for GSSOC'21 label May 7, 2021
@0xapurv
Copy link
Contributor Author

0xapurv commented May 7, 2021

Hey, @himanshusharma89, kindly review this Pr. The push notification is working and rebasing with the master branch is also done!

@@ -0,0 +1,8 @@
## This file must *NOT* be checked into Version Control Systems,
Copy link
Owner

@himanshusharma89 himanshusharma89 May 7, 2021

Choose a reason for hiding this comment

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

Don't add the auto-generated files in the PR.

@@ -6,7 +6,7 @@
additional functionality it is fine to subclass or reimplement
FlutterApplication and put your custom class here. -->
<application
android:name="io.flutter.app.FlutterApplication"
android:name=".Application"
Copy link
Owner

@himanshusharma89 himanshusharma89 May 7, 2021

Choose a reason for hiding this comment

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

The Flutter version is already greater than 1.12, so no need to integrate FCM in such a way, revert back to io.Flutter.app.FlutterApplcation

@@ -0,0 +1,18 @@
package tech.himanshusharma.relicbazaar
Copy link
Owner

Choose a reason for hiding this comment

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

The Flutter version is already greater than 1.12, so no need to integrate FCM in such a way.

@@ -6,7 +6,7 @@
additional functionality it is fine to subclass or reimplement
FlutterApplication and put your custom class here. -->
<application
android:name="io.flutter.app.FlutterApplication"
android:name=".Application"
android:label="retro_shopping"
Copy link
Owner

Choose a reason for hiding this comment

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

Update the below android:label to Relic Bazaar

@@ -33,15 +33,15 @@ class RemoteConfigService {
try {
await _remoteConfig.setDefaults(defaults);
await _fetchAndActivate();
} on FetchThrottledException catch (e) {
} on Exception catch (e) {
Copy link
Owner

Choose a reason for hiding this comment

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

Revert back to FetchThrottledException, as every other exception will be caught by the cath block below.

debugPrint('Remote config fetch throttled : $e');
} catch (e) {
debugPrint('Unable to fecth');
}
}

Future<void> _fetchAndActivate() async {
await _remoteConfig.fetch(expiration: const Duration(hours: 4));
await _remoteConfig.activateFetched();
await _remoteConfig.fetch();
Copy link
Owner

Choose a reason for hiding this comment

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

Use _remoteConfig.fetchAndActivate(), instead of 2 different command.

@@ -1,5 +1,5 @@
import 'package:advance_pdf_viewer/advance_pdf_viewer.dart';
import 'package:flutter/material.dart';
Copy link
Owner

Choose a reason for hiding this comment

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

You have done a lot of changes in this file, attach a video of this new view in the comments

firebase_remote_config: ^0.6.0
firebase_core: any
google_fonts: ^1.1.0
firebase_remote_config: ^0.10.0-dev.2
Copy link
Owner

Choose a reason for hiding this comment

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

Don't use pre-release versions.

firebase_auth: 0.20.1
native_pdf_view: ^4.0.1
firebase_auth: ^1.1.3
http_parser: ^4.0.0
Copy link
Owner

@himanshusharma89 himanshusharma89 May 7, 2021

Choose a reason for hiding this comment

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

http_parser is not being used anywhere, so remove it.

@@ -36,7 +63,45 @@ Future<void> main() async {
);
}

class MyApp extends StatelessWidget {
class MyApp extends StatefulWidget {
Copy link
Owner

Choose a reason for hiding this comment

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

Don't convert the MyApp to stateful, shift the token generation and initialization of push notification in dashboard.dart

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSSOC21 Issues created for GSSOC'21
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Cloud Messaging to create push notification
2 participants