Skip to content
This repository was archived by the owner on Nov 28, 2022. It is now read-only.

asset rewriting for default user image on setup#996

Merged
kevinansfield merged 2 commits into
masterfrom
unknown repository
Apr 3, 2018
Merged

asset rewriting for default user image on setup#996
kevinansfield merged 2 commits into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 29, 2018

When setting up my local Ghost development environment, after I ran grunt dev, I was getting "GET /ghost/assets/img/user-image.png" 404 in terminal every time I viewed step 2 of the setup process.

Upon further inspection, the image it was trying to find was /ghost/assets/img/user-image.png which didn't exactly match any other image during the setup process as the others had some sort of ID in them such as assets/img/install-welcome-3ae80f63d742f7376413a637c3d1b21f.png.

By removing style={{placeholderStyle}} and giving it a background URL within the CSS, the image is now successfully found as it's looking for /ghost/assets/img/user-image-feeb00c017f02c8915d6bc3ffdecf706.png - note that it now contains an ID, whereas it didn't before.

Functionality appears to be fine, I've tested the different instances I could think of here and screenshot the results below.

Before:
screenshot-2018-3-29 setup - ghost

After:
screenshot-2018-3-29 setup - ghost 1

After (Gravatar):
screenshot-2018-3-29 setup - ghost 2

After (Manual Upload):
screenshot-2018-3-29 setup - ghost 3

I noticed that placeholderStyle is defined in the app/components/gh-profile-image.js file... I can't see any other usages of it so if this fix is okay, would it be wise to delete references to it in that file too?

- updated the placeholder image style due to it throwing a 404 error
when going through ghost setup
@coveralls
Copy link
Copy Markdown

coveralls commented Mar 29, 2018

Coverage Status

Coverage increased (+0.005%) to 72.23% when pulling ffea40f on CriticalRespawn:fix-default-user-image into abdc3b9 on TryGhost:master.

@kevinansfield
Copy link
Copy Markdown
Member

Hey @CriticalRespawn 👋 The correct fix here is the same as the fixes in cbcf09e - the template strings are tripping up the asset rewriter so we need to split out the local asset name. We'll be getting rid of custom CSS classes soon in favour of our Tachyons based framework so it's better to keep URLs in the JS.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 2, 2018

Thanks @kevinansfield I'll take a look and try and get an updated pushed to this branch in the next few days 👍

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 3, 2018

Hi @kevinansfield 😄

I've changed this PR so it now contains the correct fix!

@ghost ghost changed the title changed placeholder image to not use placeholderStyle asset rewriting for default user image on setup Apr 3, 2018
@kevinansfield kevinansfield merged commit b96c6b9 into TryGhost:master Apr 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants