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

PR - Web icon fix #1724

Merged
merged 3 commits into from
Jul 22, 2019
Merged

PR - Web icon fix #1724

merged 3 commits into from
Jul 22, 2019

Conversation

danscime
Copy link
Collaborator

@danscime danscime commented Jul 17, 2019

Closes #1712. Closes #1633.
Website icon was not displaying due to some problems with the way the images folder was being copied.

Copy link
Owner

@JacquesCarette JacquesCarette left a comment

Choose a reason for hiding this comment

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

This is really weird. The -r meaning 'recursive' should have taken care of that. I'm kind of suspicious that something else is going on here. @Mornix , opinion?

@danscime
Copy link
Collaborator Author

Okay so 2 problems, the first being that the way I had my copy statement, the images folder was being copied into an images folder, hence images/images.

Second problem being that my icon does not seem to be under version control, I will look into why.

Once the image is pushed, I like the new way I have the copy_images function, this way we remove all images then regenerate them, that way, no old images will be stored.

@JacquesCarette
Copy link
Owner

images/images is definitely bad. Regenerating everything does seem like the Drasil way to do things.

@danscime
Copy link
Collaborator Author

Upon further inspection, it could possibly be due to different systems treating
cp -r "$CUR_DIR"website/images/ "$CUR_DIR"deploy/images
like
cp -r "$CUR_DIR"website/images "$CUR_DIR"deploy/images

the first images has a / afterwards, which could be causing a discrepancy between how my computer compiles vs. whatever system the CI uses.

Either way, I think my newer version is better as well.

@danscime
Copy link
Collaborator Author

Weird that the image wasn't being tracked online, but on my system, it said it was.
Github shows the deletion, but it never showed the original in master.
Very weird.

The new image seems to be appearing.

@Mornix
Copy link
Collaborator

Mornix commented Jul 19, 2019

@danscime's conclusions are generally correct. The way the copy_images was written the directory was being copied inside another images directory leading to images/images/.... The reason the image itself wasn't being copied, was the issue that seems to pop up occasionally relating to case-sensitivity of different filesystems. That was the case here. In the website folder there are two directories named image, one with a capital I and one without. This PR should fix both of the issues relating to the absence of a favicon.

@JacquesCarette JacquesCarette merged commit f3f9399 into master Jul 22, 2019
@JacquesCarette JacquesCarette deleted the webIcon branch July 22, 2019 16:55
@JacquesCarette
Copy link
Owner

Ok, I'll take this as a positive code review -- merged.

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

Successfully merging this pull request may close these issues.

Website icon not displaying Icon for website
3 participants