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

App linking #183

Closed
wants to merge 38 commits into from
Closed

App linking #183

wants to merge 38 commits into from

Conversation

grindlock
Copy link
Contributor

@grindlock grindlock commented May 28, 2020

Fixes #136

Description

Now to open an app from our all that needs to be done is import the component appLinkingHandler and passed url in the style show under example. A second parameter can be pass named message to show a custom text for the alert. This properly launches an app can pass parameters from our app and can show warnings if the url dont meet the standard from a single component.

  • A new field, linkedin, has been added in User.js and FormData.js for the user avaible to add their linkedin profile.
  • In dashboard the urls now reflect the new style and appLinkingHandler replaced Linking.openUrl.
  • In Profile.js and OtherProfile.js the appLinkingHandler manages the onClick for linkedin and email button.

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 4 commits May 21, 2020 04:50
…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.
…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.**
Copy link
Contributor

@HanielDiaz HanielDiaz left a comment

Choose a reason for hiding this comment

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

Overall looks like it's going in the right direction. Just 1 section with alot of complexity and a few small details to improve readability.

The reason why I'm so anal about readability is because code is read way more often than it is written, spending a lot of time and effort into writing readable code always pays off down the line when other people need to read and make changes to the code. According to many sources we spend 10 times more time reading code than we do writing it.

Copy link
Contributor

@esteban737 esteban737 left a comment

Choose a reason for hiding this comment

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

Just some brief changes to the appLinkingHandler documentation. Also as previously requested we can probably work to simplify the code so it's easier to understand and work with. Some JSDocs could also go a long way

Copy link
Contributor

@esteban737 esteban737 left a comment

Choose a reason for hiding this comment

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

Sorry for the additional pr review, but after testing the slack button link isn't working and is getting an unhandled promise rejection and there is error obtained when editing our profiles. You're going to want to merge the newest commits into your fork.

@grindlock grindlock marked this pull request as draft June 5, 2020 08:53
@@ -147,15 +148,21 @@ class OtherProfile extends Component {
socialmediarow
} = styles;

const {
Copy link
Contributor

Choose a reason for hiding this comment

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

bump

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.

Indentation, documentation, typos, and some minor recommendations - please mark these as resolved to know what was fixed.

Sergio Perez-Aponte and others added 7 commits June 7, 2020 04:59
…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.
…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 requested review from idelmr and HanielDiaz July 1, 2020 10:28
@@ -147,15 +148,21 @@ class OtherProfile extends Component {
socialmediarow
} = styles;

const {
Copy link
Contributor

Choose a reason for hiding this comment

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

bumpidump dump

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.

Reshaping final changes to the pull request

@idelmr idelmr requested review from HanielDiaz and idelmr July 4, 2020 04:49
@idelmr idelmr requested a review from HanielDiaz July 5, 2020 19:42
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.

Almost there...

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.

  • Confused about where you're storing the data for slackinfo, is it yaml or env? There might be some conflict
  • Minor enhancements

Comment on lines +3 to +12
channel:
general: CPRDXHHD4
announcements: CC5S86GHE
fundraisingCommittee: CCUUWRU1Y
motorshpe: CP5HC76TD
projects: CNATZRD9C
shpejr: GCWFE5630
mentorshpe: C016KGD2WHH
computereng: GN4U8FPDK
projectleads: GNBH9T4UC
Copy link
Contributor

Choose a reason for hiding this comment

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

  • computereng is a private channel
  • unsure about projectleads and shpejr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put them there in case the committees wanted to link to slack. Should I take them off?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine, but (1) people won't find a private channel if they're not in it and (2) we don't have all of the private channel codes, so there are a couple missing. Moreover computereng isn't a committee and I'm afraid SHPE will keep changing channels, so it would be safer to only have the default ones.

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.

I would want to test and change some minor details, but I understand you're extremely busy. I think we can push this now and change stuff later

@idelmr idelmr changed the base branch from master to appLinking February 19, 2021 06:05
@idelmr idelmr closed this Feb 19, 2021
@idelmr idelmr deleted the branch SHPEUCF:appLinking February 19, 2021 06:11
@idelmr
Copy link
Contributor

idelmr commented Feb 19, 2021

Moved to branch appLinking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open links in corresponding app
4 participants