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

Fix: Portfolio site leads to 404 #149

Closed
wants to merge 5 commits into from

Conversation

TishG
Copy link
Contributor

@TishG TishG commented Dec 25, 2021

This PR fixes...

This PR fixes #147. Appends 'https://' to the urls that do not have 'http://' or 'https://' so that the link navigates to the site instead of 404 error page.

What I did...

  • add libraries folder and utils file
  • add util function for validating and mutating the url
  • pass all urls into util function

How to test...

  1. Navigate to http://localhost:8080/about/
  2. Click 'site' for each card that contains a site button
  3. Verify that it navigates to their website and not a 404 error page
  4. Navigate to the bio page (ex: http://localhost:8080/bio/julieth-fajardo/) for each card that contains a site button
  5. Click on the 'Portfolio' link
  6. Verify that it navigates to their website and not a 404 error page

@TishG TishG requested a review from novellac December 25, 2021 00:24
@TishG
Copy link
Contributor Author

TishG commented Dec 25, 2021

@novellac
I would like you to take a look at the util function to see if it covers all the boundaries. I didn't feel it was necessary to check that the url is a string since the author is adding in a url that we are assuming will be a string with a .com at the end.

The next step is to find where to wrap the url with this function once you approve of the function.

@TishG TishG force-pushed the fix/website/147-portfolio-site-leads-to-404 branch 2 times, most recently from e681ba9 to 0208bdb Compare December 25, 2021 00:37
@TishG TishG force-pushed the fix/website/147-portfolio-site-leads-to-404 branch from 0208bdb to 78113f7 Compare December 25, 2021 00:38
@TishG TishG marked this pull request as ready for review January 4, 2022 04:45
@TishG TishG marked this pull request as draft January 11, 2022 00:36
@novellac novellac marked this pull request as ready for review February 23, 2022 01:58
Comment on lines +97 to +104
// methods: {
// validUrl
// },
// computed: {
// portfolioUrl() {
// // return this.validUrl(member.node.contact_links.portfolio_url)
// }
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Remove these unused methods.

@@ -16,13 +16,13 @@
/>
<a
v-if="$page.biopost.contact_links.email"
:href="`mailto:${$page.biopost.contact_links.email}`"
href="`mailto:${$page.biopost.contact_links.email}`"
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 here we still want the email to be dynamic, so keep the : on the href.

Comment on lines +12 to +14
* Our CMS does not validate urls,
* urls are entered in manually by the author,
* so this is to ensure a valid url to navigate to
Copy link
Contributor

Choose a reason for hiding this comment

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

Compliment: I like your explanation here, it gives great context.

Copy link
Contributor

@novellac novellac left a comment

Choose a reason for hiding this comment

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

Great job! Approved, pending resolution of the comments below.

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

Successfully merging this pull request may close these issues.

Portfolio site leads to 404
3 participants