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

adding default avatar image #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bhirmbani
Copy link

Description

I just add ternary operator to the img tag source. If it have project.Image[0].thumbnails.large.url then load it. If it doesn't then load the default avatar that is a service from adorable avatar. maybe its too cute?

Motivation and Context

I wrote this PR because Im motivated to fix the issue number #71

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@aspittel
Copy link
Owner

This is a great idea -- can we maybe move towards using the logo instead for the place holder?


Thank you so much!

@GaProgMan
Copy link
Collaborator

Added the waiting on contributor label as @aspittel has asked @bhirmbani for a small change.

@ghost
Copy link

ghost commented Nov 1, 2018

Hi @bhirmbani @GaProgMan , I don't think this will solve this issue #71 problem. Let me explain in detail:
If you beautify src/data.json file and see the "Muses Code JS":

{
    "id": "reczp03Z88eFLuJDO",
    "fields": {
      "Name": "Muses Code JS",
      "Tags": [
        "JavaScript",
        "WebDev"
      ],
      "Blog": "http://nodegirls.com.au/",
      "Website": "https://github.com/node-girls-australia",
      "About": "...... removed for simplicity",
      "Published": true
    }
  }

As you can see there is no "Image" array that contains thumbnails. So according to your index.js:

{project.Image && (
  <div class="profileImageContainer">
    <img
      src={project.Image[0].thumbnails.large.url}
      src={project.Image[0].thumbnails.large.url ? project.Image[0].thumbnails.large.url : 'https://api.adorable.io/avatars/285/learn-code-from-us-default-avtr.png'}
      alt={"Picture of " + project.Name}
      className="profileImage"
    />
  </div>
)}

Since there is no "Image" array for Musus Code JS, the <div class="profileImageContainer"> ... </div> will never run and thus there will be no profile image like before.

@bhirmbani
Copy link
Author

Ah yes, you're right. I didn't see the source. That should do the trick. But just curious, how is Musus could be no Image array at the first place? Is it possible to happening again in the future? In case of that how if we do something like the following:

import lcfu-logo.png from 'src/public/lcfu-logo.png'

{project.Image ? (
  <div class="profileImageContainer">
    <img
      src={project.Image[0].thumbnails.large.url}
      src={project.Image[0].thumbnails.large.url}
      alt={"Picture of " + project.Name}
      className="profileImage"
    />
  </div>
) : 
( <div class="profileImageContainer">
    <img
      src={project.Image[0].thumbnails.large.url}
      src={lcfu-logo.png}
      alt={"Picture of " + project.Name}
      className="profileImage"
    />
  </div>)
}

@ghost
Copy link

ghost commented Nov 1, 2018

@bhirmbani , This is a great solution. You don't even have to change data.json file. Just make sure you don't add src duplications:

<img
      src={project.Image[0].thumbnails.large.url} // Like this
      src={project.Image[0].thumbnails.large.url} // Like this
      alt={"Picture of " + project.Name}
      className="profileImage"
    />

Go ahead, I'll close my pull request #91

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

Successfully merging this pull request may close these issues.

3 participants