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

Opening links in external apps #181

Closed
wants to merge 3 commits into from
Closed

Opening links in external apps #181

wants to merge 3 commits into from

Conversation

grindlock
Copy link
Contributor

@grindlock grindlock commented May 22, 2020 •
edited by idelmr
Loading

Fixes #136

Description

Added the functionality to open the linkedin app and mail when button clicked in the profile page. In profile page code to handle app linking for social media buttons added added using uri scheme and deep linking.

  • User page under initial state added linkedin field.
  • Forms page under editProfileFormDataRegular added the linkedin field
  • Info.plist added the LSApplicationQueriesSchemes array and the whitelisted apps uri to the array.

Types of changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Checklist:

  • My code follows the code style of this project found on Contribution guidelines.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Sergio Perez-Aponte added 2 commits May 21, 2020 04:50
Reinstalled pods
731d4cb
Added Linking functionality to the social media and mail button in th…
f5a75f9 
…e profile page. In FormData page at editProfileFormDataRegular added a field call linkedin. In User page under initial state added linkedin field. In the iOS Info.plist added LSApplicationQueriesSchemes array and the whitelisted apps uri.
@grindlock
Copy link
Contributor Author

grindlock commented May 22, 2020 •
edited
Loading

I have been able to test in expo code snip environment and works fine. I was not able to test in the app due to privilege in the database. Is there a test account?

Also, ignore the "Reinstalled pods". I had to do it in my end to be able to build the app and should not be merge to the project. sorry for the inconvenience.

@grindlock grindlock closed this May 22, 2020
From previous commit to issue SHPEUCF#136 did some clean up and code …
1344ba0 
…style to profile.js. Added the functions to OtherProfile.js to handle app linking there. In FormData.js jus did code clean up to the element added earlier in edit regular profile. ** in profile in form component in the values parameter I erase function that was formating the this.props.activeuser since it was creating a type missmatch in Form.js. the parameter values is expecting an object but was receiving an array. this cause the app to crash when onsubmit was called.**
@grindlock grindlock reopened this May 22, 2020
@grindlock
Copy link
Contributor Author

I reopen the pull request after fixing some of the code and updated. Also found a problem that break the app when user click edit profile and then click submit. The problem is Profile under form when passing the value. In form the parameter values is expecting an object but is pass is an array. this is due to formatting the output to an array before passing it.

idelmr
idelmr suggested changes May 26, 2020
View reviewed changes
Copy link
Contributor

@idelmr idelmr left a comment

Choose a reason for hiding this comment

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

Some styling, formatting, and grammar issues.

  1. Try to keep your commit messages in active voice and present tense
  2. Keep commit messages less than 2 sentences long
  3. Make sure that you're paying attention to all the linter rules

src/data/FormData.js
@@ -161,11 +161,10 @@ const editProfileFormDataRegular = [
}
},
{
placeholder: "LinkedIn Profile",
placeholder: "LinkedIn Profile (after the in/)",
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be another way of specifying this, as a non-technical user might not know what "after the in/" means

src/screens/User/OtherProfile.js
Comment on lines 162 to 164
onPress = { () => {
Alert.alert("Coming Soon");
// Actions.PostShow({ title: 'Linkedin', uri: 'https://www.linkedin.com/'})
this.openApp(linkedin, firstName);
} }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this onPress into one line?

src/screens/User/OtherProfile.js
Comment on lines 171 to 173
onPress = { () => {
Alert.alert("Coming Soon");
// Actions.PostShow({ title: 'Github', uri: 'https://www.github.com/'})
this.openEmail(email, firstName);
} } >
Copy link
Contributor

Choose a reason for hiding this comment

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

One line as well

src/screens/User/OtherProfile.js

async openApp(profile, name) {
if (!profile) {
Alert.alert(`${ name } has not added his LinkedIn profile yet.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to "...has not added their LinkedIn profile yet"

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces around curly braces when using interpolation

src/screens/User/OtherProfile.js
let url = `linkedin://in/${ profile }`;

console.log(`URL: ${ profile }`);
// then have to check if the link can be handle
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unnecessary comment

src/screens/User/OtherProfile.js
@@ -174,6 +178,42 @@ class OtherProfile extends Component {
</View>
);
}

async openApp(profile, name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also specify app being opened

src/screens/User/OtherProfile.js
Alert.alert(`${ name } has not added his LinkedIn profile yet.`);
}
else {
let url = `linkedin://in/${ profile }`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spaces around curly braces

src/screens/User/OtherProfile.js
if (canOpenLink)
await Linking.openURL(url);
else
await Linking.openURL(`https://linkedin.com/in/${ profile }`);
Copy link
Contributor

Choose a reason for hiding this comment

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

No spaces around curly braces

src/screens/User/OtherProfile.js

async openEmail(to, name) {
if (!to) {
Alert.alert(`No email for ${ name } has been found.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using the type: "error" for alert. Also, remove spacing around curly braces for interpolation

src/screens/User/OtherProfile.js
Alert.alert(`No email for ${ name } has been found.`);
}
else {
let url = `mailto:${ to }`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove spacing around curly brace

@idelmr idelmr changed the title This is for the "Open links in corresponding app #136" issue Opening links in external apps May 26, 2020
@idelmr idelmr requested review from HanielDiaz and esteban737 May 27, 2020 01:19
@idelmr
Copy link
Contributor

idelmr commented May 28, 2020

Submitting another PR soon

@idelmr idelmr closed this May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idelmr idelmr idelmr requested changes

@HanielDiaz HanielDiaz Awaiting requested review from HanielDiaz

@esteban737 esteban737 Awaiting requested review from esteban737

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

Successfully merging this pull request may close these issues.

Open links in corresponding app
2 participants
@grindlock @idelmr