Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

improvement: Expands Short Links #381

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Conversation

redreceipt
Copy link
Contributor

@redreceipt redreceipt commented Nov 13, 2019

DESCRIPTION

Screen Shot 2019-11-13 at 9 20 03 AM

What does this PR do, or why is it needed?

Will try to expand slug as a short link first to get the content

How do I test this PR?

Click different links to make sure they open in the app. On alpha, a short link you can use is 'emmaguthrie'.

TODO

  • I am affirming this is my best work (Ecclesiastes 9:10)
  • PR has a relevant title that will be understandable in a public changelog (ie...non developers)
  • No new warnings in tests, in storybook, and in-app
  • Upload GIF(s) of iOS and Android if applicable
  • Set a relevant reviewer

REVIEW

  • Review updates to test coverage and snapshots
  • Review code through the lens of being concise, simple, and well-documented

Manual QA

  • Manual QA on iOS and ensure it looks/behaves as expected
  • Manual QA on Android and ensure it looks/behaves as expected

The purpose of PR Review is to improve the quality of the software.

@redreceipt redreceipt added the ready for review This is ready to be reviewd label Nov 13, 2019
@codecov-io
Copy link

codecov-io commented Nov 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@1054f37). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             develop    #381   +/-   ##
=========================================
  Coverage           ?   44.1%           
=========================================
  Files              ?     174           
  Lines              ?    1712           
  Branches           ?     187           
=========================================
  Hits               ?     755           
  Misses             ?     841           
  Partials           ?     116
Impacted Files Coverage Δ
...s-church-api/src/data/content-items/data-source.js 16.1% <0%> (ø)

@richarddubay
Copy link
Contributor

richarddubay commented Nov 19, 2019

This doesn't seem to open links like https://ns.link/emmaguthrie in the app on Android. The reason is that there is no route in the AndroidManifest.xml file for / and therefore never tries to open the app to get the slug. That's easy enough to add and I've tested it locally and it works, so I could add it here.

A bigger, more concerning issue is opening links like that in iOS. Android is easy because we allow all the links to come through and then handle them in the AndroidManifest.xml file. iOS doesn't work that way. Apple wants us to define all the routes up front in the apple-app-site-association file. Which means that if we want routes like this to work, we'd need to allow the / route ... I think. I don't feel great about that because then it would allow any and all links.

Thoughts? Is there a downside to just letting all the links through? Am I just thinking about this the wrong way?

@redreceipt redreceipt mentioned this pull request Nov 21, 2019
9 tasks
@redreceipt redreceipt added the blocked Blocked by another PR label Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked Blocked by another PR ready for review This is ready to be reviewd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants