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

Books plugin documentation update #60

Merged
merged 13 commits into from
Jul 23, 2024
Merged

Books plugin documentation update #60

merged 13 commits into from
Jul 23, 2024

Conversation

S-Haime
Copy link
Member

@S-Haime S-Haime commented Mar 5, 2024

Documentation for the books plugin.

Adding @joemull as a reviewer since you volunteered and @ajrbyers as this is your plugin.

Minor points:

  • Monthly reporting is currently broken in the plugin. Issue is open, but this is therefor not included (but will be similar to regular metrics).
  • What is the recommended size for books covers? Same as journal covers?
  • Currently missing the Sphinx files and conf.py (I understand how it works in theory as its documented well enough + can see how it works on JW main and imports, but it might be good for me to see someone do it first, before messing about).

Copy link
Member

@joemull joemull left a comment

Choose a reason for hiding this comment

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

Thanks @S-Haime--this is really good! I especially like the contextual detail you provide. Things like this show you are thinking through the questions people will have, which of course you are, but just wanted to say so explicitly because it is great:

Make sure that the filename of the file uploaded is consistent and correct. Whilst Janeway will change the filename to the title internally, depending on the application used to open the document after download, the original filename might still be visible. Google Chrome is an example of an application that might still display the original filename in its reader toolbar, as displayed in the image below.

I added a few comments below.

In terms of building the documentation, I don't think we have a setup for RST on plugins yet. We'd have to configure that if we wanted it to show up on readthedocs.io. I can do that and help you with it @S-Haime so if that's what we want to do. Thoughts @ajrbyers?

Another option: Since we are phasing out RST, one easy short-term solution that keeps us from having to add more RST configuration is to just put this in a markdown file, something like USER_GUIDE.md at the top level of the repository. With an images folder replacing nstatic and a link from the main README.md file so people can read the docs on GitHub. Basic but functional. What do you think?

docs/Books.rst Outdated
- No

.. [#] Using a character encoding other than UTF-8 can cause bugs during imports or updates. `(What is character encoding?) <https://www.w3.org/International/questions/qa-what-is-encoding>`_. These apps save .csvs with UTF-8 by default: OpenRefine, LibreOffice, Google Sheets, and Apple Numbers. However! If you use Microsoft Excel, keep in mind some versions don’t automatically create .csv files with UTF-8 character encoding. This may cause punctuation and special characters to be garbled on import. So, when saving, look for the ‘.csv (UTF-8)’ option in the drop-down box.
.. [#] Required due to a bug - we aim to fix this in the near future.
Copy link
Member

Choose a reason for hiding this comment

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

Might be a missing reference to this note?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now in .md

Copy link
Member

Choose a reason for hiding this comment

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

These images are great in terms of the content they cover and their frequency.

But for the long term I'm hoping we can take narrower versions of them, so they fit in a narrower container on our future docs site. We currently let the text and images run very wide in our documentation, but this isn't best practice and our future documentation site will hopefully use something more in the range of <580px for prose and <960px for content.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I haven't addressed any of the images, as I imagine we'll want to look at this as part of the website building.)

Copy link
Member

@ajrbyers ajrbyers 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 great. As @joemull mentioned we are phasing rst out so it may be worth converting this to md. For this file, as there are no toctrees it should be relatively simple.

docs/Books.rst Outdated Show resolved Hide resolved
@joemull
Copy link
Member

joemull commented Mar 12, 2024

Thanks @ajrbyers.

@S-Haime I think that means we should do this:

Another option: Put this in a markdown file, something like USER_GUIDE.md at the top level of the repository. With an images folder replacing nstatic and a link from the main README.md file so people can read the docs on GitHub.

Let me know if I can help. I will go ahead and give this PR the status of Changes Requested.

@S-Haime S-Haime requested review from joemull and ajrbyers July 18, 2024 06:47
@S-Haime
Copy link
Member Author

S-Haime commented Jul 18, 2024

The .rst file can be retired at some point now :)

@joemull joemull assigned joemull and unassigned S-Haime Jul 18, 2024
@joemull joemull removed the request for review from ajrbyers July 18, 2024 09:07
@joemull joemull requested a review from ajrbyers July 18, 2024 09:22
@joemull joemull assigned ajrbyers and unassigned joemull Jul 18, 2024
@joemull
Copy link
Member

joemull commented Jul 18, 2024

Thanks @S-Haime -- looks good to me. I added another commit to delete "Books.rst," if that's alright with you. Over to you for the second green tick @ajrbyers.

@ajrbyers ajrbyers merged commit e852baf into master Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants