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: make challenge links on profile page work #36701

Merged

Conversation

moT01
Copy link
Member

@moT01 moT01 commented Aug 27, 2019

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the master branch of freeCodeCamp.
  • None of my changes are plagiarized from another source without proper attribution.
  • All the files I changed are in the same world language (for example: only English changes, or only Chinese changes, etc.)
  • My changes do not use shortened URLs or affiliate links.

Closes #36085

@ojeytonwilliams
Copy link
Contributor

I'll take a proper look at this soon, but my first impressions are that this is a nice solution. The extra work the server has to do is pretty small.

I am still wondering if we can get away with not using the server at all. Nonetheless, if we are using it, this looks like the way to go.

@raisedadead raisedadead added platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. release: next/beta type: showstopper Issues that are urgent and are critical path. These need immediate attention & shipping. labels Aug 28, 2019
@raisedadead raisedadead added this to In progress in Next via automation Aug 28, 2019
@moT01
Copy link
Member Author

moT01 commented Aug 28, 2019

hey @raisedadead - I noticed that the certifications are part of the table on the profile, is this deliberate? Screen Shot 2019-08-28 at 7 29 26 AM

@raisedadead
Copy link
Member

We can update the styles. It was sort of a mess. Feel free to take a dig at it.

@ojeytonwilliams
Copy link
Contributor

@raisedadead The issue is that the TimeLine's cert links are currently 404s, so we were wondering if they should be filtered out or made to point at the certifications.

Also, @RandellDawson noticed that the certifications appear in the TimeLine even if they aren't public. Presumably that's a bug!

@raisedadead
Copy link
Member

🤦‍♂ I need more coffee. Yes, this looks like a bug.

@moT01 moT01 marked this pull request as ready for review August 28, 2019 18:23
@moT01 moT01 requested a review from a team August 28, 2019 18:23
@ahmaxed
Copy link
Member

ahmaxed commented Aug 28, 2019

it seems like we are moving in the direction of completely removing these links and making the the profile more resume like. I wonder if we should sunset this list now if fixing it is too time consuming. @raisedadead thoughts?

@raisedadead
Copy link
Member

it seems like we are moving in the direction of completely removing these links and making the the profile more resume like. I wonder if we should sunset this list now if fixing it is too time consuming.

I think this should be fixed now.

We have not planned or designed anything about the "resume" like settings page. It could presumably take weeks. From my point of view this is a blocker issue right now from releasing master.

I would defer to @QuincyLarson though.

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

I have a couple of nitpicks, but otherwise this looks good.

client/src/components/profile/components/TimeLine.js Outdated Show resolved Hide resolved
client/src/components/profile/components/TimeLine.js Outdated Show resolved Hide resolved
client/src/components/profile/components/TimeLine.js Outdated Show resolved Hide resolved
@ojeytonwilliams ojeytonwilliams added the status: merge conflict To be applied to PR's that have a merge conflict and need updating label Aug 29, 2019
@raisedadead raisedadead merged commit 70b7080 into freeCodeCamp:master Aug 29, 2019
Next automation moved this from In progress to Done Aug 29, 2019
@moT01 moT01 deleted the fix/challenge-links-on-profile-page branch August 29, 2019 18:12
@moT01
Copy link
Member Author

moT01 commented Aug 29, 2019

Thanks for all the reviews lately everyone! 🎉 @raisedadead @ahmadabdolsaheb @RandellDawson @ojeytonwilliams @thecodingaviator @Manish-Giri @jonathan-grah 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. status: merge conflict To be applied to PR's that have a merge conflict and need updating type: showstopper Issues that are urgent and are critical path. These need immediate attention & shipping.
Projects
No open projects
Next
  
Done
Development

Successfully merging this pull request may close these issues.

Broken links on profile page
4 participants