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(client): show certs on timeline #37947

Merged

Conversation

@ojeytonwilliams
Copy link
Contributor

ojeytonwilliams commented Dec 17, 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.

This provides idToNameMap with the information it needs to pass on to the timeline - namely the id, title and path of the certifications.

TODO:

  • make the timeline's certification entries visually distinct

I refactored the selector into the same module as the rest of the selectors, when I thought it would be useful, but left it there since it seems like a better place for it.

This does duplicate the cert info again, so will need refactoring down the line. I think, when the api-server is next overhauled, all the cert info should be moved into one module that's common to both client and server.

Closes #37943

@gitpod-io

This comment has been minimized.

Copy link

gitpod-io bot commented Dec 17, 2019

@moT01

This comment has been minimized.

Copy link
Member

moT01 commented Dec 23, 2019

I played around with a few different icons and ways to make certs stand out a little. This is what I came up with that I think I like the best...
Screen Shot 2019-12-23 at 7 28 37 AM
Screen Shot 2019-12-23 at 7 31 26 AM

What do you think @ojeytonwilliams?

Edit: I added these changes to the PR

@ojeytonwilliams

This comment has been minimized.

Copy link
Contributor Author

ojeytonwilliams commented Dec 23, 2019

I think this looks great. It should be pretty obvious what the rosettes stand for, but they fit nicely with the overall aesthetic.

@ahmadabdolsaheb what do you reckon?

@raisedadead

This comment has been minimized.

Copy link
Member

raisedadead commented Dec 23, 2019

This is amazing nice work both of you!

@ahmadabdolsaheb

This comment has been minimized.

Copy link
Member

ahmadabdolsaheb commented Dec 25, 2019

Thanks for the pr. The icon looks good and simple. Down the line we could unify all of our icons to make sure they look and feel similar to one another.

As @moT01 mentioned, we should only show the certificate only when the certificates are set to public from the settings

@raisedadead

This comment has been minimized.

Copy link
Member

raisedadead commented Dec 30, 2019

Are we waiting on something?

@moT01 moT01 marked this pull request as ready for review Dec 30, 2019
@moT01 moT01 requested a review from freeCodeCamp/dev-team as a code owner Dec 30, 2019
@moT01

This comment has been minimized.

Copy link
Member

moT01 commented Dec 30, 2019

I'm pretty sure it's good to go.

Copy link
Member

raisedadead left a comment

Will it be possible for this to be moved over to the common utils:

client/src/components/profile/utils/index.js

@ojeytonwilliams

This comment has been minimized.

Copy link
Contributor Author

ojeytonwilliams commented Dec 31, 2019

When I checked this out locally, the new cert didn't appear until I reloaded the timeline. Other than that (maybe something to fix later) I think everything's fine.

@raisedadead

This comment has been minimized.

Copy link
Member

raisedadead commented Dec 31, 2019

I am not sure about the filename to be index, but we can worry about it later. Hey @ahmadabdolsaheb can you take a look and merge this when you get a chance.

@raisedadead raisedadead changed the title Fix(client): show certs on timeline fix(client): show certs on timeline Jan 3, 2020
@raisedadead raisedadead merged commit 1a56f4d into freeCodeCamp:master Jan 3, 2020
2 checks passed
2 checks passed
Gitpod Open an online workspace in Gitpod
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.