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

fixed Open links of About page in browser #270

Closed

sainideepanshu199 wants to merge 11 commits into CircuitVerse:master from sainideepanshu199:easy_fix

sainideepanshu199 commented Feb 27, 2023 •

edited

Loading

Fixes #259

Describe the changes you have made in this PR -
fixed email and slack opening links to default email app and browser respectively.

Screenshots of the changes (If any) -

Note: Please check Allow edits from maintainers. if you would like us to assist in the PR.


          fixed email opening and join Slack button bug , issues #259

a6cb87c

sainideepanshu199 changed the title ~~fixed Open links of About page in browser~~ fixed Open links of About page and Contributors Page in browser

tanmoysrt suggested changes

View reviewed changes

lib/ui/components/cv_social_card.dart Outdated

Comment on lines 26 to 31

+                      if (choice == 1) {
+                        final url = Uri.parse('mailto:support@circuitverse.org');
+                        await launchUrl(url);
+                      } else {
+                        launchURL(url);
+                      }

Member

tanmoysrt Feb 28, 2023

You can consider to manage this from the function itself rather than overriding it here and adding new argument.
You can override it in the lib/utils/url_launcher.dart , launchURL function , to check whether the url starts with mailto, if match you can parse the uri and call required function

lib/ui/views/about/about_view.dart Outdated

@@ @@ -108,12 +108,14 @@ class _AboutViewState extends State<AboutView> { @@
                               title: AppLocalizations.of(context)!.email_us_at,
                               description: 'support@circuitverse.org',
                               url: 'mailto:support@circuitverse.org',
+                              choice: 1,

Member

tanmoysrt Feb 28, 2023

Remove this argument, as it is required only to check mail

lib/ui/views/about/about_view.dart Outdated

                             ),
                             CircuitVerseSocialCard(
                               imagePath: 'assets/images/contribute/slack.png',
                               title: AppLocalizations.of(context)!.join_slack,
                               description: AppLocalizations.of(context)!.slack_channel,
                               url: 'https://circuitverse.org/slack',
+                              choice: 2,

Member

tanmoysrt Feb 28, 2023

Remove this argument, as it is required only to check mail

lib/ui/views/contributors/contributors_view.dart Outdated

@@ @@ -29,19 +29,21 @@ class ContributorsView extends StatelessWidget { @@
                             title: 'Email us at',
                             description: 'support@circuitverse.org',
                             url: 'mailto:support@circuitverse.org',
+                            choice: 1,

Member

tanmoysrt Feb 28, 2023

Remove this argument, as it is required only to check mail

lib/ui/views/contributors/contributors_view.dart Outdated

                           ),
                           CircuitVerseSocialCard(
                             imagePath: 'assets/images/contribute/slack.png',
                             title: 'Join and chat with us at',
                             description: 'Slack channel',
                             url: 'https://circuitverse.org/slack',
+                            choice: 2,

Member

tanmoysrt Feb 28, 2023

Remove this argument, as it is required only to check mail

lib/ui/views/contributors/contributors_view.dart Outdated

    
                              title: 'Contribute to open source',

                              description: 'Github',

                              url: 'https://github.com/CircuitVerse',

                              choice: 3),

Member

tanmoysrt Feb 28, 2023

Remove this argument, as it is required only to check mail

lib/utils/url_launcher.dart Outdated

Comment on lines 3 to 8

    
              void launchURL(String url) async {

              Future<void> launchURL(String url) async {

                if (await canLaunchUrlString(url)) {

                  await launchUrlString(url);

                  await launchUrlString(url, mode: LaunchMode.externalApplication);

                } else {

                  throw 'Could not launch $url';

                }

Member

tanmoysrt Feb 28, 2023

Everything seems fine ! Check whether the url starts with mailto and take action on that.

Author

sainideepanshu199 Mar 1, 2023

I have fixed the use of extra parameter's and function and made changes accordingly. Now mail and slack link on about page can open in mail app and browser respectively with just the url_launcher utils. please review the code and suggest if any other changes can be made.

vedant-jain03 suggested changes

View reviewed changes

Member

vedant-jain03 left a comment

@sainideepanshu199 You can't work on 2 issues at a time, also #260 has been assigned to other contributor, kindly remove the required code to fix #260 issue!

sainideepanshu199 changed the title ~~fixed Open links of About page and Contributors Page in browser~~ fixed Open links of About page in browser

sainideepanshu199 added 5 commits

March 1, 2023 14:53


          removed the specific function call for mail


          removed the extra parameter and unused imports

8747dfc


          removed the extra parameter

4a28014


          removed the extra parameters not useful for bug fix #259

992f0f4


          added function to open mail and other links

df0e9da

mails to open with default mail app 
and other links to open in default browser

Author

sainideepanshu199 commented Mar 1, 2023

@sainideepanshu199 You can't work on 2 issues at a time, also #260 has been assigned to other contributor, kindly remove the required code to fix #260 issue!

I have removed the code required to fix #260 issue

sainideepanshu199 requested review from vedant-jain03 and tanmoysrt and removed request for vedant-jain03 and tanmoysrt

March 1, 2023 11:49

vedant-jain03 requested a review from tanmoysrt

March 1, 2023 11:55

tanmoysrt reviewed

View reviewed changes

lib/utils/url_launcher.dart Outdated

Comment on lines 5 to 6

		var str1 = 'mailto';
		if (url.startsWith(str1)) {

Member

tanmoysrt Mar 1, 2023

You can directly write the string inside startswith(...) as str1 is not used anywhere else

Author

sainideepanshu199 Mar 1, 2023

used the string directly and removed str1.


          remove the extra string declaration

5a4e734

Member

tanmoysrt commented Mar 1, 2023

@sainideepanshu199 run dart format . to format for your code.


           formatted the code

Member

tanmoysrt commented Mar 1, 2023

@sainideepanshu199 great 👍

Author

sainideepanshu199 commented Mar 2, 2023

@sainideepanshu199 great 👍

Can u please review the code

tanmoysrt approved these changes

View reviewed changes

Member

tanmoysrt left a comment

Fine !

Member

tanmoysrt commented Mar 2, 2023

@sainideepanshu199 please wait a bit. The CI pipeline has some issue, so it can delay the merge a bit.

Author

sainideepanshu199 commented Mar 2, 2023

@sainideepanshu199 please wait a bit. The CI pipeline has some issue, so it can delay the merge a bit.

can i work on issue #261 as it is not assigned to anyone.

Member

tanmoysrt commented Mar 2, 2023

@sainideepanshu199 please wait a bit. The CI pipeline has some issue, so it can delay the merge a bit.

can i work on issue #261 as it is not assigned to anyone.

Comment on the specfic issue for get assigned

vedant-jain03 suggested changes

View reviewed changes

Member

vedant-jain03 left a comment

@Tanmoy741127 Do we need to update pubspec.lock, if not @sainideepanshu199 kindly remove that change from commit!

Member

tanmoysrt commented Mar 4, 2023

@Tanmoy741127 Do we need to update pubspec.lock, if not @sainideepanshu199 kindly remove that change from commit!

It's not needed, @sainideepanshu199 remove the file from commit

Member

manjotsidhu commented Mar 4, 2023

@Tanmoy741127 Can you verify launchUrl function is only called from these links, we don't want to open externally any other links.

Member

tanmoysrt commented Mar 4, 2023

@Tanmoy741127 Can you verify launchUrl function is only called from these links, we don't want to open externally any other links.

Ok ! as per the updates, it is going to update all links externally, so that behaviour need to be changed

tanmoysrt suggested changes

View reviewed changes

lib/utils/url_launcher.dart Outdated

                 } else {
-                  throw 'Could not launch $url';
+                  await launchUrlString(url, mode: LaunchMode.externalApplication);

Member

tanmoysrt Mar 4, 2023

Only use LaunchMode.externalApplication for this urls

In any other case open in the app webview itself

Author

sainideepanshu199 commented Mar 5, 2023

@Tanmoy741127 Do we need to update pubspec.lock, if not @sainideepanshu199 kindly remove that change from commit!

It's not needed, @sainideepanshu199 remove the file from commit

do i need to delete it from my commit?


           fixed open in app webview issue

Author

sainideepanshu199 commented Mar 5, 2023

now only mail, slack and github opens in external application rest opens in app webview

please review and tell for any further changes

Member

tanmoysrt commented Mar 5, 2023

now only mail, slack and github opens in external application rest opens in app webview

please review and tell for any further changes

Will check soon.


           added bool value to lauch in external application at all function oc…

a95848e

…curances

sainideepanshu199 closed this

sainideepanshu199 reopened this

tanmoysrt reviewed

View reviewed changes

lib/utils/url_launcher.dart

Comment on lines +6 to +7

		final url = Uri.parse('mailto:support@circuitverse.org');
		await launchUrl(url);

Member

tanmoysrt Mar 5, 2023

For mail consider to open in external application.

Author

sainideepanshu199 Mar 5, 2023

yes it opens in default mail app of smartphone

Author

sainideepanshu199 Mar 5, 2023

i have sent you a video regarding all links on slack, please watch it , it will clear all doubts

Member

tanmoysrt Mar 5, 2023

Ok then it's fine

Author

sainideepanshu199 Mar 5, 2023

@sainideepanshu199 remove the pubspec.lock file

shall i delete the file or revert the changes

Member

tanmoysrt Mar 5, 2023

just delete the file and commit. It will work.

Author

sainideepanshu199 Mar 5, 2023

done

Member

tanmoysrt commented Mar 5, 2023

@sainideepanshu199 remove the pubspec.lock file


          removed pubspec.lock file

7c7e267

Member

tanmoysrt commented Mar 5, 2023

cc: @manjotsidhu is any other changes required ?

Member

manjotsidhu commented Mar 5, 2023

There are merge conflicts

sainideepanshu199 closed this

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

Labels

None yet