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

Help buttons #945

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Help buttons #945

merged 2 commits into from
Mar 21, 2024

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Mar 19, 2024

Rationale

Fix #889
Fix #890

Changes

  • Modify GET /offliner/{offliner} endpoint:
    • Move array to flags property (it is generally considered a bad design to return arrays as root JSON objects anyway)
    • Add a help property which is the URL of the help to configure given offliner
      • Always computed as https://github.com/openzim/{offliner}/wiki/Frequently-Asked-Questions for now, but could be more specific later on, at least for few offliners
      • Could be reused in other frontend (e.g. zimit-frontend), hence adding it to the backend (rather than the frontend) makes sense
Before After
image image
  • Add 3 help buttons and two dividers
Before After
image image

@benoit74 benoit74 self-assigned this Mar 19, 2024
@benoit74
Copy link
Collaborator Author

@kelson42 @Popolechien does the new "Help" buttons in the UI match your expectations? FYI, clicking the button will open the link in a new browser tab

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.98%. Comparing base (fd56855) to head (8d7d176).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #945      +/-   ##
==========================================
+ Coverage   87.84%   87.98%   +0.13%     
==========================================
  Files          93       94       +1     
  Lines        5307     5327      +20     
==========================================
+ Hits         4662     4687      +25     
+ Misses        645      640       -5     

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

@benoit74
Copy link
Collaborator Author

FYI again, the Help button in "Content settings" section leads to https://github.com/openzim/zimfarm/wiki/Recipe-configuration-%E2%80%90-Content-settings and the Help button in "Task settings" section leads to https://github.com/openzim/zimfarm/wiki/Recipe-configuration-%E2%80%90-Task-settings. Both are freshly created and mostly placeholders for now.

@benoit74 benoit74 marked this pull request as ready for review March 19, 2024 14:25
@benoit74 benoit74 requested a review from rgaudin March 19, 2024 14:25
@Popolechien
Copy link
Contributor

does the new "Help" buttons in the UI match your expectations

LGTM

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Thank you ;
Something bothers me with the new label (Content/Task/Scrapper) but I can't find any better ones and those headers improve readiness 👍

@benoit74
Copy link
Collaborator Author

Something bothers me with the new label (Content/Task/Scrapper) but I can't find any better ones and those headers improve readiness 👍

They bother me as well, but I can't find better ones either! ^^

@Popolechien
Copy link
Contributor

Popolechien commented Mar 19, 2024

Some of the fields should probably be moved about so that they better match the content / task and scraper titles (periodicity should move to Task for instance; Scraper settings could be Name settings as we have now (and remove Command flags so as to keep the title consistent)

@benoit74
Copy link
Collaborator Author

Some of the fields should probably be moved about so that they better match the content / task and scraper titles (periodicity should move to Task for instance; Scraper settings could be Name settings as we have now (and remove Command flags so as to keep the title consistent)

@Popolechien I don't mind if you want to move fields, but this is a new issue, please create one with a more precise proposition than "some ... should ..." and the reasoning behind it. Since this current PR / change LGTY, I will merge it for now, we can always continue to enhance it afterwards.

@benoit74 benoit74 merged commit 818cc1a into main Mar 21, 2024
7 checks passed
@benoit74 benoit74 deleted the help_buttons branch March 21, 2024 07:36
@Popolechien
Copy link
Contributor

@benoit74 Yeah it wasn't an actual request but more a generic response to the "Something bothers me with the new label" comment. Thanks.

@kelson42
Copy link
Contributor

@benoit74 thx for the PR... I share the "bad feeling" expressed

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