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

Look for environment-specific UIO landing URLs first #397

Merged
merged 3 commits into from
Aug 19, 2021
Merged

Look for environment-specific UIO landing URLs first #397

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c352d4c
Look for environment-specific UIO landing URLs first
kalvinwang Aug 19, 2021
dcd5420
refactor
kalvinwang Aug 19, 2021
4aa562e
Merge branch 'main' into kalvin-landing-urls
kalvinwang Aug 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ relates to weekly certification
- CERTIFICATE_DIR: The path to the client certificate (certificate must be in PFX/P12 format)
- PFX_FILE: The name of the client certificate file
- (Optional) PFX_PASSPHRASE: The import passphrase for the client certificate if there is one
- (Optional) URL_UIO_LANDING, URL_UIOMOBILE_LANDING: The environment-specific links to the UIO landing page
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- (Optional) URL_UIO_LANDING, URL_UIOMOBILE_LANDING: The environment-specific links to the UIO landing page
- (Optional) URL_UIO_LANDING, URL_UIOMOBILE_LANDING: The environment-specific links to the UIO landing pages


For local development:

Expand Down
6 changes: 5 additions & 1 deletion components/TransLine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ export interface TransLineProps extends TransLineContent {
function resolveUrl(link: I18nString, userArrivedFromUioMobile: boolean) {
// Special case for UIO homepage links.
if (link === 'uio-home') {
const uioHomeLink = userArrivedFromUioMobile ? getUrl('uio-home-url-mobile') : getUrl('uio-home-url-desktop')
// Optional environment-specific links back to the UIO landing page, used by EDD testing
const uioLandingUrl = process.env.URL_UIO_LANDING ?? getUrl('uio-home-url-desktop')
const uioMobileLandingUrl = process.env.URL_UIOMOBILE_LANDING ?? getUrl('uio-home-url-mobile')

const uioHomeLink = userArrivedFromUioMobile ? uioMobileLandingUrl : uioLandingUrl
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Not necessary for launch, but I think this is enough logic to put it somewhere else (like in getUrl.ts) rather than in the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I should have put it there. Moved!

if (uioHomeLink) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rocketnova I don't understand this if statement-- is it to protect against the possibility of uio-home-url-desktop/mobile, not existing as a string? If they do exist, then uioHomeLink will always be true right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Typescript will yell if we don't narrow here, because the code doesn't know those json strings definitely exist - we have a ticket to see if there's some way to assert the json strings do exist #338

Copy link
Contributor

Choose a reason for hiding this comment

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

getUrl() can return undefined. I'm not positive what happens if public/urls.json disappears or is malformed or has a read-error, but there's definitely the possibility that public/urls.json exists and the linkKey passed in doesn't exist in the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Typescript will yell if we don't narrow here, because the code doesn't know those json strings definitely exist - we have a ticket to see if there's some way to assert the json strings do exist #338

Agreed that we should know at build time, but I think the check is important in case something horrible and unexpected happens at runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, this all makes sense. Thanks!

// If the link is for UIO homepage, do a direct getUrl() lookup.
// Do not pass the looked up url through t() because t() will mangle the url.
Expand Down