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

Fix typos and inconsistencies in docs files #21

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

adrienave
Copy link
Contributor

This PR addresses a few typos/errors I found while reading the docs for the first time, as I wanted to understand better how this application is working.

Please check carefully my changes because I might have overlooked some details/done some mistakes, since I didn't use this application on a real scale yet.

I also take the opportunity to list some suggestions for amelioration, don't hesitate to tell me if I should convert them to a single issue, a bunch of issues or even fix some in this PR.

  • Default papi-web.ini content specifies host and port properties with : instead of =, which looks like the standard according to docs.
  • Menus section (34) doesn't specify that menu_text is required for the screen to be displayed as a menu, it was misleading for me at first, since menu property is introduced first, but adding only it to my screens for testing was not changing anything in the application, nor displaying any warning/error in the dedicated space. This information is specified in ref section (40) though.
  • I feel like template & families section (33) could be inverted with menus section (34), since templating is an harder concept to understand (imo) and this section rely heavily on stuff from menus section (not only menu and menu_text, but also %t, %f and %l). Though menus section also rely on templating through examples, but these examples could be adapted maybe.
  • Not directly related to docs, but why not providing meaningful labels for the different steps of a timer (section 31), like before, soon and after (instead of 1, 2 and 3).
  • Rotators section (35) doesn't emphase that families property can take a list of families, I thought families was just a misleading name, and noticed later (in ref section 40) that it can be also a list.

PS: These are just some neat pickings, it's truly amazing you built such a dense, clean and easy to follow documentation, lots of softwares should take example on this, thank you!

@Amaras
Copy link
Collaborator

Amaras commented Feb 11, 2024

Hi, thanks for the PR, I'll try to review it as soon as possible, but probably not until Monday 19.

As for the points you've made:

  • You can probably correct the style inconsistency in papi-web.ini in this PR;
  • That's an issue with the docs, indeed: you'd see the warnings in the logs, but it's not clear enough in the relevant section… Feel free to open an issue about that;
  • Makes sense to me, should probably be an issue as well;
  • I think we could add descriptive names in addition to 1, 2 and 3, I don't think we're ready to deprecate yet… This should be an issue;
  • We should explain that correctly, yeah…

Since they're all documentation-related, I think you should open only one issue with different tasks.

Thanks again for the PR, feel free to contribute to the code if you want to do so :)

@adrienave
Copy link
Contributor Author

You can probably correct the style inconsistency in papi-web.ini in this PR;

I don't think I can ; I'm talking about the default configuration provided as part of the latest build version (https://github.com/papi-web-org/papi-web/releases/tag/2.1.6).
I can't find any occurrence of either papi-web.ini or host: 0.0.0.0 in the project that would be related to the generation of this file, I believe it's some manual action done at release time.

[...] you'd see the warnings in the logs [...]

I didn't see any in the dedicated page in the web application, but maybe it is present in the terminal logs?
Or maybe my logging level is wrong.

I think we could add descriptive names in addition to 1, 2 and 3, I don't think we're ready to deprecate yet… This should be an issue;

Right, how did I forget about backward compatibility...

So sure, I will open an issue for all these points!

Thanks again for the PR, feel free to contribute to the code if you want to do so :)

That's what I'm considering yes, but I didn't start investigating the code yet.

Copy link
Collaborator

@pascalaubry pascalaubry left a comment

Choose a reason for hiding this comment

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

Really impressed by the quality of your feedback, really precious for our users.

@pascalaubry pascalaubry merged commit 4461438 into papi-web-org:dev Feb 13, 2024
@adrienave adrienave deleted the fix-docs branch February 13, 2024 08:41
@adrienave adrienave mentioned this pull request Feb 14, 2024
3 tasks
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.

None yet

3 participants