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

[Docs] Edit docs for readability part 1 #337

Merged
merged 38 commits into from
Feb 29, 2024

Conversation

stichbury
Copy link
Contributor

@stichbury stichbury commented Feb 27, 2024

Description

This is the first of several PRs to work through the docs for readability and improve consistency.

In this one, I've done this:

  • capitalised all the headings
  • reorganised the presentation of the installation content (the page is now under "Get started)
  • done a line-by-line edit on the tutorials section to make sure the style guide is (mostly) applied.

Please take a look through at the built docs since the edits are going to be horrid to read .

I'd particularly like feedback on section 3 as I rewrote that and added some new material about parameters/selectors. I also fixed a "bug" in the code where it was applying the second opacity parameter to both charts (when the docs said only the first chart).

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@stichbury stichbury self-assigned this Feb 27, 2024
@stichbury stichbury added the Issue: Docs 🗒️ Issue for markdown and API documentation label Feb 27, 2024
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

Hey @stichbury ,

I took a quick look - it looks good to me in general, thanks for cleaning up!

I have some general questions:

  • These nested admonitions look a bit funky to me - are these intended?
  • I've noticed that you've used "How to..." in several sections now. I actually don't have any issues with that, but I remember that we used to follow a guideline to not use How to that often, but only in the first title. I think it came from this docs guideline that @maxschulz-COL sent you as a link? Happy to follow with whatever you think is best though
  • I left some suggestion where I wasn't sure why we kept it capitalized

The rest looks great though! 🚀

vizro-core/docs/pages/user-guides/custom-components.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user-guides/data.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user-guides/data.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user-guides/data.md Show resolved Hide resolved
vizro-core/docs/pages/user-guides/filters.md Show resolved Hide resolved
vizro-core/docs/pages/user-guides/install.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user-guides/install.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/tutorials/explore-components.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/tutorials/explore-components.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/tutorials/explore-components.md Outdated Show resolved Hide resolved
@stichbury
Copy link
Contributor Author

Thanks @huong-li-nguyen !

You've been very diligent and applied some of the edits I made to pages I haven't reached yet. Thanks for doing that, but don't worry too much on those pages...I'll get there eventually.

  • These nested admonitions look a bit funky to me - are these intended?

I did intend them, yes. Let's see what people think. I wanted to put side-information in the aside, but then found there was side-information about the side-information (like how to install pipenv if you don't want to use conda). I thought they looked OK but let's see what the general view is. I can always remove them!

  • I've noticed that you've used "How to..." in several sections now. I actually don't have any issues with that, but I remember that we used to follow a guideline to not use How to that often, but only in the first title. I think it came from this docs guideline that @maxschulz-COL sent you as a link? Happy to follow with whatever you think is best though
  • I left some suggestion where I wasn't sure why we kept it capitalized
    Ah, that's because I've just corrected the subheads for now rather than all the sections for the admonitions, except where I did the line edit in the Get Started section. I didn't want to make the PR too big in terms of diffs to review but then I guess it means the built docs look weird where I haven't edited in full. Thanks for catching 🎈

stichbury and others added 4 commits February 27, 2024 14:38
Co-authored-by: Li Nguyen <90609403+huong-li-nguyen@users.noreply.github.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Li Nguyen <90609403+huong-li-nguyen@users.noreply.github.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Li Nguyen <90609403+huong-li-nguyen@users.noreply.github.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Li Nguyen <90609403+huong-li-nguyen@users.noreply.github.com>
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@Joseph-Perkins
Copy link
Contributor

For the "How to..." sections, I think the decision process around them before was related to how they affect the sub-navigation menus. In cases where people might want to quickly run their eyes down a list of topics in a menu, to decide what to click on for finding out more information, then it seems easier to run eyes over the list and take in information when the "How to" part is not included

However when acting as a section header in the main body of the documentation, then having the "How to" prefix can be more informative to readers

So I suppose our decision making criteria would be on a case-by-case basis, depending on which navigation menu the title is being used in, and how important it is to be able to run eyes down that quickly

@stichbury
Copy link
Contributor Author

So I suppose our decision making criteria would be on a case-by-case basis, depending on which navigation menu the title is being used in, and how important it is to be able to run eyes down that quickly

That makes sense. The docs have great structure per the diataxis model (they're all "How to" guides) but they're called 'User guides' in the navigation and I think we need to clarify that they are practical content rather than reference/theoretical explanations. I'll pay some attention to where the "How to" is showing up to keep the scan overhead for the user to a minimum.

I wonder if we should rename the section from User guides to How to guides on the card and the top horizontal nav.

WDYT? @maxschulz-COL

Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

Thanks for incorporating the changes! 🚀

@huong-li-nguyen huong-li-nguyen changed the title [docs] Edit docs for readability part 1 [Docs] Edit docs for readability part 1 Feb 28, 2024
@stichbury stichbury enabled auto-merge (squash) February 28, 2024 15:28
.gitignore Outdated Show resolved Hide resolved
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
.gitignore Outdated Show resolved Hide resolved
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@astrojuanlu
Copy link

GitHub tells me that I've been mentioned in this PR but I don't see where 😅

@stichbury
Copy link
Contributor Author

stichbury commented Feb 28, 2024

GitHub tells me that I've been mentioned in this PR but I don't see where 😅

@astrojuanlu Thanks for dropping by! 👋

Lol, it was here: https://github.com/mckinsey/vizro/pull/337/files/ddce1007f8007f45cc21afe65b75d183fe418ff9#r1505838867

image

We were wondering if the instructions in the Kedro docs for using pipenv or venv instead of conda were valid, since I added them to Vizro in overhauling the installation docs. I've since removed them again because I figure, in the case of Vizro at least, that novice users don't need to know about them, and more experienced users will likely already have their preferred virtual environment tool. I've just linked them instead.

@astrojuanlu
Copy link

For more context on my thoughts on Kedro installation instructions, see kedro-org/kedro#3281

Long story short, I think "native" methods (pip) should be shown on top, then conda as an alternative. And please don't mention Pipenv, unless you're also mentioning Poetry, PDM, Hatch, and Rye.

@antonymilne
Copy link
Contributor

Cool, my spidey sense that this guidance would not be sanctioned by @astrojuanlu were right 😀 I have exactly the same feelings as him on the topic so fully agree with kedro-org/kedro#3281.

@stichbury
Copy link
Contributor Author

Cool, my spidey sense that this guidance would not be sanctioned by @astrojuanlu were right 😀 I have exactly the same feelings as him on the topic so fully agree with kedro-org/kedro#3281.

In the context of this PR however, the question for me is -- who needs what guidance?

I'm assuming that experienced Python users already know and understand the need for a virtual environment and have their preferred way of using one. So we don't need to address them (but we can point to somewhere that describes the options if it helps -- hit me with a preferred link).

  • So who needs it? Probably inexperienced users, right? We need to make it easy for them.
  • What guidance should we give them? What shall we say that doesn't set you chaps off on your holy war, but is straightforward enough for new users? I still think conda is a good choice because it's easy to explain but I'm open to any other suggestions. Tell me what you want, I'll write it! 😆

@antonymilne
Copy link
Contributor

antonymilne commented Feb 29, 2024

I've pushed a commit 8e222fe to update this:

  • mention pip install right at the beginning of the page, because that's really the crucial step that applies to all readers
  • update virtual environment guidance to mention venv first, then conda, and nothing else

Here's the relevant page: https://vizro--337.org.readthedocs.build/en/337/pages/user-guides/install/, which is easier to read than the commit diff.

lmk what you think @astrojuanlu and feel free to put this in the kedro docs too if you like it.

@stichbury
Copy link
Contributor Author

I've pushed a commit 8e222fe to update this:

Nice, thanks! I think that's a satisfactory outcome for all reader types. I'll had considered that we could adjust the bit about Anaconda navigator to bring that up the page for those that want to avoid the whole scenario of virtual envs, but that takes them into the Anaconda docs and introduces unnecessary complexity. This is a nice compromise -- thank you!

@stichbury
Copy link
Contributor Author

When you've a moment, please could I get an approval @antonymilne or @maxschulz-COL and I'll get this merged so I can get on with the next set of edits.

@stichbury stichbury merged commit c06dfa5 into main Feb 29, 2024
35 checks passed
@stichbury stichbury deleted the docs/light-edit-for-readability branch February 29, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Docs 🗒️ Issue for markdown and API documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants