Show profile image for logged in user #111
Conversation
|
@adamstankiewicz @georgebabey This is a new PR. Initial review was completed at #106. Needs your final review. |
muhammad-ammar
force-pushed
the
ammar/ENT-1104-profile-image
branch
from
c9df25c
to
e84bb23
Compare
src/components/Header/index.jsx
Outdated
| @@ -50,12 +66,18 @@ Header.propTypes = { | |||
| enterpriseLogo: PropTypes.string, | |||
| enterpriseName: PropTypes.string, | |||
| email: PropTypes.string, | |||
| username: PropTypes.string, | |||
| fetchUserProfile: () => {}, | |||
There was a problem hiding this comment.
Looks like propTypes and defaultProps are reversed for fetchUserProfile
| profile: { | ||
| profile_image: { | ||
| has_image: true, | ||
| image_url_medium: 'https://raw.githubusercontent.com/edx/edx-portal/master/src/images/edx-logo.png', |
There was a problem hiding this comment.
We should import this image instead of using the url from Github. Don't know if I trust a https://raw.githubusercontent.com link will always stick around.
Related, the test name could just say "renders profile image correctly" as we don't really care if it's an external image or not here.
There was a problem hiding this comment.
I didn't mention it as its a) in a test and b) I don't think it will actually fetch the image, would just render the snapshot with a reference to it so if it was no longer there the test would pass. But it might not hurt to make it obvious its a dummy url like https://path-to-profile-image
There was a problem hiding this comment.
That's true it wouldn't actually be fetching the image in the snapshot, in which case a dummy URL would work.
But, if we decide to eventually use visual regression testing/diffing, it would be fetched so we might as well import and use the actual edx-logo.png image here.
muhammad-ammar
force-pushed
the
ammar/ENT-1104-profile-image
branch
from
e84bb23
to
706c18d
Compare
|
@adamstankiewicz @georgebabey Addressed the feedback. Please continue the review. |
ENT-1104