Skip to content
This repository was archived by the owner on Jul 3, 2025. It is now read-only.

Feature/positions redirect ssr#30

Closed
bronz3beard wants to merge 6 commits intostagingfrom
feature/positions-redirect-ssr
Closed

Feature/positions redirect ssr#30
bronz3beard wants to merge 6 commits intostagingfrom
feature/positions-redirect-ssr

Conversation

@bronz3beard
Copy link
Copy Markdown
Contributor

resolves #4

  • Transfer Positions to this new architecture (add redirectMessage component to new architecture)

Reference of original issue from website-v4 'Redirect links to missing job listings to jobs? #768'

Reference for original PR 'Feature/positions missing redirect v2 #923'

Testing link:
/positions/reco6YkMgTONSB3mS/hoodie-day-apparel-ambassador

This should have the same behaviour as website-v4 and should replicate all PR review changes, if anything is missing from previous PR let me know.

@bronz3beard bronz3beard temporarily deployed to website-v5-feature-posi-g7fqmj October 29, 2019 06:49 Inactive
@bronz3beard bronz3beard changed the base branch from master to staging October 29, 2019 06:49
Comment thread src/pages/positions/index.js Outdated
}, [props])

const getCurrentUrl = () => {
const currentURL = window.location.pathname.split('/')[1];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

for me, the process terminates when I just go to /positions, because ReferenceError: window is not defined here. Not sure what's up with that – @bronz3beard, does it not happen for you?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

… because for server-side rendering, there is no window … 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think on the second render it should exist client side, it didn't cause me an issue but I will try to get it sorted 😃 so it works for us all!!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it didn't cause you an issue because probably you navigate to positions, so it was always on client-side...if you reload positions, even better a hard reload, it should throw the error...we need to validate if window variable exists...so you can do:
if (typeof window !== 'undefined') { ... }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I couldn't reproduce the error in firefox/chrome but either way i should check for window! thanks team.

Copy link
Copy Markdown
Contributor

@kbardi kbardi left a comment

Choose a reason for hiding this comment

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

Looks good but I found some pending validations that could generate some errors, and some improvements.

Comment thread src/components/jobs/index.js Outdated
Comment thread src/components/jobs/index.js Outdated
Comment thread src/components/jobs/index.js Outdated
Comment thread src/components/jobs/index.js Outdated
Comment thread src/components/jobs/index.js Outdated
Comment thread src/components/positionsRedirectMessage/redirectMessage.js Outdated
Comment thread src/pages/positions/index.js Outdated
}, [props])

const getCurrentUrl = () => {
const currentURL = window.location.pathname.split('/')[1];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it didn't cause you an issue because probably you navigate to positions, so it was always on client-side...if you reload positions, even better a hard reload, it should throw the error...we need to validate if window variable exists...so you can do:
if (typeof window !== 'undefined') { ... }

Comment thread src/pages/positionsEntry/index.js Outdated
Comment thread src/pages/positionsEntry/index.js Outdated
Comment thread src/pages/positionsEntry/index.js Outdated
@bronz3beard bronz3beard temporarily deployed to website-v5-feature-posi-g7fqmj October 31, 2019 02:25 Inactive
@bronz3beard bronz3beard temporarily deployed to website-v5-feature-posi-g7fqmj November 4, 2019 00:18 Inactive
@bronz3beard bronz3beard temporarily deployed to website-v5-feature-posi-g7fqmj November 4, 2019 22:09 Inactive
@camposcristian camposcristian removed their request for review November 18, 2019 04:30
@camposcristian
Copy link
Copy Markdown
Contributor

@kbardi once you merge #31 could you speak with Rory so we can transfer this work to the new architecture

kbardi added a commit that referenced this pull request Nov 21, 2019
@kbardi
Copy link
Copy Markdown
Contributor

kbardi commented Nov 22, 2019

I close it because I merged it on the new architecture (branch feature/donate-with-nextjs)

@kbardi kbardi closed this Nov 22, 2019
@kbardi kbardi deleted the feature/positions-redirect-ssr branch March 9, 2020 20:06
camposcristian pushed a commit that referenced this pull request Jul 17, 2020
camposcristian pushed a commit that referenced this pull request Jul 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants