Skip to content

fix: add close button to about overlay - #137

Open
1cbyc wants to merge 1 commit into
openzim:mainfrom
1cbyc:fix/about-close-button
Open

1cbyc wants to merge 1 commit into
openzim:mainfrom
1cbyc:fix/about-close-button

Conversation

@1cbyc

@1cbyc 1cbyc commented Aug 19, 2026

Copy link
Copy Markdown

Summary

  • Add an explicit close button to the About overlay
  • Keep the existing history/back button behavior intact
  • Cover the generated About HTML in the processor tests

Test Plan

  • cd scraper && .venv/bin/python -m pytest -q tests/test_processor.py -k 'about_html'
  • cd scraper && .venv/bin/ruff check src/maps2zim/processor.py tests/test_processor.py
  • cd scraper && .venv/bin/ruff format --check src/maps2zim/processor.py tests/test_processor.py
  • git diff --check

Fixes #134

Co-authored-by: insisong emmanuelisaacnsisong@gmail.com

@benoit74
benoit74 self-requested a review August 20, 2026 05:24

@benoit74 benoit74 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I need more input from @kelson42

The "about" is not an overlay/popup but a dedicated page (which might look like an overlay/popup, I get it). So the "close button" implemented in this PR is a trick which navigates back to the home page. This is a trick, but since this is the only page the user might come from, it works. Is this what you expected or would you prefer we move the about to a real popup, since this also has the advantage that we do not have to decide about a background color anymore? I feel like this would make more sense

@benoit74

benoit74 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

@kelson42 up

@kelson42

kelson42 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

I need more input from @kelson42

The "about" is not an overlay/popup but a dedicated page (which might look like an overlay/popup, I get it). So the "close button" implemented in this PR is a trick which navigates back to the home page. This is a trick, but since this is the only page the user might come from, it works. Is this what you expected or would you prefer we move the about to a real popup, since this also has the advantage that we do not have to decide about a background color anymore? I feel like this would make more sense

I'm OK with both approaches... even if a proper dialog with a blured background would be neater visually.

I think we should go with current approach. It does the requested job.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 35.88%. Comparing base (3c3cdbb) to head (01d3495).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #137   +/-   ##
=======================================
  Coverage   35.88%   35.88%           
=======================================
  Files          11       11           
  Lines        1045     1045           
  Branches      153      153           
=======================================
  Hits          375      375           
  Misses        654      654           
  Partials       16       16           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@benoit74

Copy link
Copy Markdown
Collaborator

@kelson42 OK thank you

@1cbyc please remove this merge commit from main to feature branch and rather rebase on top of main ; merging from main to feature branch is against our policy

@1cbyc
1cbyc force-pushed the fix/about-close-button branch from 0819699 to 6ed5200 Compare September 10, 2026 10:33
@1cbyc

1cbyc commented Sep 10, 2026

Copy link
Copy Markdown
Author

Rebased onto the current main branch and force-pushed. The branch now contains a single feature commit with no merge commit.

@benoit74
benoit74 self-requested a review September 10, 2026 15:22

@benoit74 benoit74 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add an entry in CHANGELOG.md (sorry for missing this earlier) and I will merge.

Thank you!

@1cbyc

1cbyc commented Sep 10, 2026

Copy link
Copy Markdown
Author

Added the requested entry under the Unreleased section of CHANGELOG.md. The branch remains cleanly rebased with the feature commit and this small follow-up commit.

Co-authored-by: insisong <emmanuelisaacnsisong@gmail.com>
@1cbyc
1cbyc force-pushed the fix/about-close-button branch from 3cd57ad to 01d3495 Compare September 10, 2026 20:27
@1cbyc

1cbyc commented Sep 10, 2026

Copy link
Copy Markdown
Author

@benoit74 I've squashed the commits as requested into a single feature commit. Let me know if you need anything else!

@benoit74
benoit74 self-requested a review September 10, 2026 20:49

@benoit74 benoit74 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, thank you!

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.

"About" should have a close button

3 participants