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

Fixed (Favicon related) errors which returned by Zimcheck #175

Merged
merged 12 commits into from
Dec 28, 2022

Conversation

pavel-karatsiuba
Copy link
Collaborator

Fix favicons links
Remove broken links
Fix the "redundant data" error after Zimcheck

Fixes #141

@pavel-karatsiuba pavel-karatsiuba marked this pull request as ready for review December 14, 2022 17:47
Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

I don't have tested, but from the principle it looks good. But can you please secure the images behind the link are good. Here this is always the same stuff used. It should not. If you take the logo of the welcome page, you should be able to recreate good one in all resolution needed easily.

Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

I'm not sure my last comment has been understood clearly, here is my test result:
image

The Android does not pass, and I believe actually the favicon.ico shoudl not be used for many of the other links.

@kelson42 kelson42 changed the title Fixed errors which returned by Zimcheck Fixed (Favicon related) errors which returned by Zimcheck Dec 23, 2022
@kelson42 kelson42 force-pushed the issue-141-fix-icons branch 2 times, most recently from e45dd0b to 0d2db8b Compare December 24, 2022 07:25
Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

I have started the review of this PR, but there is already a first regression: the ZIM logo/favicon is not there anymore, see the question mark:
image

Actually the zìmcheck does detect it:

$ zimcheck dist/phet_
[INFO] Checking zim file dist/phet_
[INFO] Zimcheck version is 3.1.3
[INFO] Verifying ZIM-archive structure integrity...
error 2 opening file "dist/phet_
kelson@camber:~/code/phet$ zimcheck dist/phet_fr_2022-12.zim 
[INFO] Checking zim file dist/phet_fr_2022-12.zim
[INFO] Zimcheck version is 3.1.3
[INFO] Verifying ZIM-archive structure integrity...
[INFO] Avoiding redundant checksum test (already performed by the integrity check).
[INFO] Searching for metadata entries...
[INFO] Searching for Favicon...
[INFO] Searching for main page...
[INFO] Verifying Articles' content...
[INFO] Searching for redundant articles...
  Verifying Similar Articles for redundancies...
[INFO] Checking for redirect loops...
[ERROR] Favicon:
  Favicon is missing
[INFO] Overall Test Status: Fail
[INFO] Total time taken by zimcheck: 3 seconds.

Please fix and introduce a zimcheck test run on the ZIM created with the test, you can look how this is done already at https://github.com/openzim/mwoffliner/tree/main/test

Favicon checker looks good! But there is a problem with safari-pinned-tab.svg which is full black.
image

Fix favicons links
Remove broken links
Fix "redundant data" error after zimcheck
Added favicons with different sizes
Added favicons with different sizes
Tests were fixed because were removed some lines from index.html
favicon fixes
zimcheck added into tests
lint fixes
Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

zimcheck does not run in the CI, it does not work like on MWoffliner (given as example in my comment). I have fixed this, but the CI does not pass, because there is still a problem around the ZIM file with the dist.js file. I'm not sure for what that file exactly was used to, but it is not part anymore of the git repository. Please fix and explain why this is really not necessary anymore (I don't get it).

The rest looks good. Thx for the fixes.

Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

LGTM

@kelson42 kelson42 merged commit 5ea8ced into main Dec 28, 2022
@kelson42 kelson42 deleted the issue-141-fix-icons branch December 28, 2022 18:56
pavel-karatsiuba pushed a commit that referenced this pull request Dec 28, 2022
Fixed (Favicon related) errors which returned by Zimcheck
pavel-karatsiuba added a commit that referenced this pull request Dec 28, 2022
This reverts commit d66cbe8, reversing
changes made to e212586.
pavel-karatsiuba pushed a commit that referenced this pull request Dec 28, 2022
Fixed (Favicon related) errors which returned by Zimcheck
pavel-karatsiuba added a commit that referenced this pull request Dec 29, 2022
This reverts commit 6d541e2, reversing
changes made to 4889617.
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.

Zimcheck returns errors: Broken links of favicon.ico
2 participants