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

new docs, complete #3369

Closed
wants to merge 5 commits into from
Closed

new docs, complete #3369

wants to merge 5 commits into from

Conversation

lazarusA
Copy link

This updates de current docs template.

  • Feature missing: versioning, to be addressed on a different PR, if this is proposal is good enough.

This is how it looks like, once the site is build:

light

light

dark

dark

@bkamins bkamins added the doc label Aug 12, 2023
@bkamins bkamins added this to the 1.7 milestone Aug 12, 2023
@bkamins
Copy link
Member

bkamins commented Aug 12, 2023

Thank you! I will ping the community on Slack for the feedback.

My question would be how the width of the middle section scales as the window is resized horizontally? (also how the new template behaves on mobile). Maybe some reference docs that use a similar template can be x-linked so that users can play with it?

Now we have something like:

image

which is not optimal.

@lazarusA
Copy link
Author

The template is responsive on all devices. Usually we should have a (link) preview per PR but I don't see one for this PR (it looks like external PRs don't get deployed).

Similar templates are:

https://juliadatacubes.github.io/YAXArrays.jl/dev/
https://tidierorg.github.io/TidierData.jl/latest/
https://rafaqz.github.io/DimensionalData.jl/dev/

and also some loca previews for some devices:
wide

mobile tablet

@bkamins
Copy link
Member

bkamins commented Aug 13, 2023

Feature missing: versioning, to be addressed on a different PR

This can be a separate PR, but I just wanted to make sure how it would work. In particular - we will have old docs in a different format and new docs in a new format. Would these integrate or not?

@lazarusA
Copy link
Author

the way it is now links to old docs will not be integrated. However, once versioning is working the idea is to have them integrated, as in

https://www.evetion.nl/SpaceLiDAR.jl/dev/

although, at the moment I still haven't figured out the right workflow to achieve this.

@bkamins
Copy link
Member

bkamins commented Aug 13, 2023

maybe just adding a link to old docs somewhere would suffice? Thank you for working on it.

@lazarusA
Copy link
Author

Yes, a link will be the easiest solution. Maybe another badge linking the old docs, to version 1.6 I suppose. https://dataframes.juliadata.org/v1.6/

@bkamins bkamins requested a review from nalimilan August 13, 2023 11:08
Manifest.toml
.DS_Store
.vscode

/Manifest.toml
Copy link
Member

Choose a reason for hiding this comment

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

Manifest.toml (also below) and .vscode are duplicate.
We do not use deps, tmp, generated and build AFAICT. Why do you add them?

Copy link
Author

Choose a reason for hiding this comment

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

  • tmp so that my local tmp attempts don't get committed, deps should go
  • generated is also generated in local builds, and build should go also I think.

JULIA_DEBUG: "Documenter"
DATADEPS_ALWAYS_ACCEPT: true
run: |
julia --code-coverage=user --project=docs/ --color=yes docs/make.jl
Copy link
Member

Choose a reason for hiding this comment

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

why add --code-coverage=user? This is the default, right? Is it used for anything in this build?

Copy link
Author

Choose a reason for hiding this comment

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

Is it used for anything in this build?
nope. It should go.

docs/make.jl Outdated Show resolved Hide resolved
docs/make.jl Outdated
],
strict = true
clean=true,
doctest=false,
Copy link
Member

Choose a reason for hiding this comment

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

why do you set doctest=false?

Copy link
Author

Choose a reason for hiding this comment

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

some tests were failing on main, and in order to build the initial site I set this to false. I agree that it should be true but my concern was not about fixing those issues and rather having the template up.

clean=true,
doctest=false,
sitename="DataFrames.jl",
authors="Bogumił Kamiński et al.",
Copy link
Member

Choose a reason for hiding this comment

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

@lazarusA - thank you for putting this 😄, but we need some wider agreement on the authors. @nalimilan - what do you think we should have here? Maybe JuliaData et at. (or indeed putting some names?)

:linkcheck,
:parse_error,
:example_block,
# Other available options are
Copy link
Member

Choose a reason for hiding this comment

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

while we are at it I would try to turn on as many checks as possible (and if some fail try fixing this, unless for some reason it is not possible)

Copy link
Author

Choose a reason for hiding this comment

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

if any of them fail, building the docs is not possible. That's why I set the test one to false.

theme:
name: material
logo: assets/logo.png
features:
Copy link
Member

Choose a reason for hiding this comment

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

are there any of the features that are debatable (i.e. that it would be worth to decide if we turn them on/off - there are many of them and for most I do not understand in 100% what they do 😄)?

Copy link
Author

Choose a reason for hiding this comment

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

I only use the ones that make sense to me 😄 . And I left the others just in case someone wants to play with these settings.

- "Functions": "./lib/functions.md"
- "Indexing": "./lib/indexing.md"
- "Metadata": "./lib/metadata.md"
# hide("Internals" => "lib/internals.md")
Copy link
Member

Choose a reason for hiding this comment

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

why hide is not needed any more? I would assume that Documenter.jl would error if we do not cover this file in docs with hide.

Copy link
Author

Choose a reason for hiding this comment

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

as it is, it just outputs a warning saying that this is file is available but not in the menu bar.

Copy link
Author

Choose a reason for hiding this comment

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

lalonso@pc DataFrames.jl % cd docs 
lalonso@pc docs % mkdocs serve
INFO     -  Building documentation...
INFO     -  Cleaning site directory
INFO     -  The following pages exist in the docs directory, but are not included in the "nav"
            configuration:
              - assets/README.md
              - lib/internals.md
INFO     -  Documentation built in 2.54 seconds
INFO     -  [13:44:20] Watching paths for changes: 'docs', 'mkdocs.yml'
INFO     -  [13:44:20] Serving on http://127.0.0.1:8000/


This resource aims to teach you everything you need to know to get up and
running with tabular data manipulation using the DataFrames.jl package.
Welcome to the `DataFrames.jl `documentation!
Copy link
Member

Choose a reason for hiding this comment

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

why backquote DataFrames.jl here and below? This is not code. We consistently use package names in plain text in the docs/docstrings in DataFrames.jl.

Copy link
Author

Choose a reason for hiding this comment

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

ok. If that's the convention, it should be like that. For me these ones here are just a guide to the eye. I will fix this.

@@ -64,8 +76,8 @@ a list of well-supported libraries that provide different data science tools,
along with a few notes about what makes each library special, and how well
integrated they are with DataFrames.jl.


- **Statistics**
```@raw html
Copy link
Member

Choose a reason for hiding this comment

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

  1. why this triplebackquote with @raw html is needed?
  2. what does ??? tip command do?

Copy link
Author

Choose a reason for hiding this comment

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

raw and ??? are to have an expandable menu.

@@ -0,0 +1,16 @@
window.MathJax = {
Copy link
Member

Choose a reason for hiding this comment

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

This is very good! We avoided LaTeX in the docs, but now we can use it.

@bkamins
Copy link
Member

bkamins commented Aug 13, 2023

I left some comments/questions as probably I do not know everything I should know 😄. Thank you!

@lazarusA
Copy link
Author

this is the output for raw and ???

tip

lazarusA and others added 2 commits August 13, 2023 13:48
Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>
@lazarusA
Copy link
Author

thanks for the feedback, latest commit should address them. Except authorship 😄 .
Thanks for taking the time to review.

@digital-carver
Copy link

Looking at TidierData docs, the font sizes of text inside the expandable ??? sections seem to be several sizes smaller than the text of the rest of the page, making them harder to read. (Same goes for the "Top-level macros" and "Helper functions" admonition-sections in the second half of that page.) Will that be the case here too, and is there any way to avoid that font size reduction?

@bkamins
Copy link
Member

bkamins commented Aug 14, 2023

I have checked this both on laptop and on mobile and they are readable. Having said that indeed they are several sizes smaller. @lazarusA - maybe making them only a bit smaller than the main text would be better?
(I assume making them smaller is intentional)

@lazarusA
Copy link
Author

@bkamins yes, being smaller is intentional and is the default, but now with the latest commit we can change it if we want. I increased the fontsize a little bit, now is just 1px below the main text. IMO now is big/distinguishable enough.

@bkamins
Copy link
Member

bkamins commented Aug 16, 2023

@lazarusA - we have reviewed and discussed this PR internally. The short conclusion is the following:

  • we believe that look&feel you propose is better than what we have; so this is a plus 😄
  • at the same time we want to make sure that we will not run into issues with long term maintenance of the documentation (note that for this package we really need to think in 10-years ahead mode); so this is a minus 😞

The concrete potential problems we see are:

  • PR introduces DocumenterMarkdown.jl dependency, and this package documentation states that it is not actively maintained; last patch release was 2 years ago.
  • PR introduces dependencies on Python and multiple Python packages; it is not clear for us if there is a guarantee that these packages would be actively maintained in the long-term and that the integration would keep working.
  • No one of the core contributors knows the newly introduced ecosystem (we know how to build and deploy docs with Documenter.jl only); this means that we would need information on several potential maintainers of this part of the package, if we merged the PR, so that if something stops working in the future there is someone with the understanding how things work and how to fix them.
  • It was not fully clear to us if the new template supports forward-versioning (we understand it does not support backward versioning, but does it support switching versions for the future releases); also, in particular, since current versioning relies on URL e.g. https://dataframes.juliadata.org/v1.3/ for latest patch release of 1.3 version it was not fully clear why it cannot be supported out of the box (it seems that general versioning would just switch to the old version of the documentation).

@lazarusA could you please comment on how you see these issues? I know you have put a lot of effort into this PR, however, we need to be sure that merging it will not run into technical issues that will outweigh the benefits of better display.

@lazarusA
Copy link
Author

could you please comment

I will not. Thanks for taking the time. I, don't have more time to get into a back and forth discussion.

@lazarusA lazarusA closed this Aug 16, 2023
@lazarusA lazarusA deleted the docs_proposal branch August 16, 2023 11:52
@bkamins
Copy link
Member

bkamins commented Aug 16, 2023

Thank you for working on the PR.

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

Successfully merging this pull request may close these issues.

3 participants