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.

Sign up for GitHub

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

Jump to bottom

Show per page documentation #13376

Merged
merged 1 commit into from Jan 23, 2020
Merged

Show per page documentation #13376

merged 1 commit into from Jan 23, 2020

Conversation

marusak
Copy link
Member

@marusak marusak commented Jan 10, 2020 •
edited

To see OS specific documentation, you need to use also ws from this PR.

helpisinthetown

Followup:

@marusak marusak added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jan 10, 2020
@andreasn
Copy link
Contributor

We discussed this in a call earlier today, but leaving a comment here as well for transparency.

  1. The most high level docs first, and then more and more granular sounds good to me, so general OS docs first.
  2. Generic items only are best I think. A page can have an unknown amount of short subsections, so it's best to do that navigation on the docs site itself, where you get more context. For very specific subjects, such as joining a realm or creating a network bond we can link to those specific docs inline, such inside the domain or bonding dialogs.

@andreasn
Copy link
Contributor

Looks generally good to me, but for some reason, it doesn't pick up the OS docs on my fedora system.

@andreasn
Copy link
Contributor

Are we happy with just Logs and Services for the initial implementation? Can always add other links later.

@marusak
Copy link
Member Author

marusak commented Jan 10, 2020

it doesn't pick up the OS docs on my fedora system.

"To see OS specific documentation, you need to use also ws from this PR."

Are we happy with just Logs and Services for the initial implementation?

No, I'll add all of them (by "all" I of course mean the best I can do, but someone may also have some suggestions. There is no limit on how many and which docs we have on each page)

@martinpitt
Copy link
Member

  1. My preference is that the Help menu goes from most specific to least specific. I. e. on the Services page, the topmost entry is actually the help for that page, then general web console, then general OS. I don't have a strong opinion about this, though.

  2. Agreed with Andreas, the per-page menu should not have too many items, at most one page specific one. Help for more specific details should then be put into dialogs, or overview cards, or e. g. the "LVM" box on the Storage page and such.

@andreasn
Copy link
Contributor

I feel the if it goes from most-to-least specific you have items jump around a lot, depending if that page has specific help or not. But I don't have any super-strong feelings about the order in general.

martinpitt
martinpitt reviewed Jan 10, 2020
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Very nice! I'm glad that making the Help menu dynamic per-page is reasonably straightforward!

doc/guide/packages.xml Outdated Show resolved Hide resolved
doc/guide/packages.xml Outdated Show resolved Hide resolved
pkg/systemd/manifest.json.in
"docs": [
{
"name": "Services",
"url": "https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/managing_systems_using_the_rhel_8_web_console/managing-services-in-the-web-console_system-management-using-the-rhel-8-web-console"
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit sceptical that this is a stable link. Did you talk to Vendula about that? I think she had a solution for that.

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 did talk to her and this indeed is a stable link. It is constructed from chapters and sections IDs.

@andreasn andreasn mentioned this pull request Jan 10, 2020
Closed
@marusak marusak removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jan 13, 2020
@marusak
Copy link
Member Author

marusak commented Jan 13, 2020

Went through the documentation and put related chapters to related pages. Also wrote tests. Ready for review. (running tests to see, if they work on all OSes, or if there are some surprises).

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Jan 13, 2020
@martinpitt
HACKING: Document how to use locally built cockpit-ws
4c3ff36 
This is useful for validating PRs like cockpit-project#13376
@martinpitt martinpitt mentioned this pull request Jan 13, 2020
Merged
martinpitt added a commit that referenced this pull request Jan 13, 2020
@martinpitt
HACKING: Document how to use locally built cockpit-ws
624dc8a 
This is useful for validating PRs like #13376

Closes #13389
martinpitt
martinpitt requested changes Jan 13, 2020
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

This looks amazing, thanks! Just some nitpicks.

doc/guide/packages.xml Outdated Show resolved Hide resolved
pkg/shell/indexes.js Outdated Show resolved Hide resolved
test/verify/check-pages Outdated Show resolved Hide resolved
test/verify/check-pages
# DOCUMENTATION_URL is only in Fedora
# RHEL is tracked in rhbz#1789984
if "fedora" in m.image:
expected = "Fedora documentation" + expected
Copy link
Member

Choose a reason for hiding this comment

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

Curious that this works -- so wait_collected_text() ignores all space? Like this it will look for "documentationWeb Console", which is confusing. Could this get a space? Likewise in teh "".join() above?

Copy link
Member Author

Choose a reason for hiding this comment

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

The method is already implemented like that and we use it on a few places (also in podman).
It literally just picks all text from all matching elements. It does not add any spaces or anything if text does not contain it.

@andreasn
Copy link
Contributor

Tested again with all the new links. Works great, thanks!

@marusak marusak mentioned this pull request Jan 22, 2020
Merged
martinpitt
martinpitt previously approved these changes Jan 23, 2020
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Cheers! 👍

@martinpitt
Copy link
Member

@marusak: Did you deliberately re-trigger the two @Bots things? I thought they were from yesterday's version of the PR?

@marusak
Copy link
Member Author

marusak commented Jan 23, 2020

Did you deliberately re-trigger the two @Bots things?

Yes I did, I wanted to tested it today.

@marusak
Copy link
Member Author

marusak commented Jan 23, 2020

Needs to be skipped on rhel-8-2-distropkg (totally forgot we introduced this in a meantime)

@martinpitt martinpitt merged commit 3ff4bed into cockpit-project:master Jan 23, 2020
@marusak marusak mentioned this pull request Feb 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpitt martinpitt martinpitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@marusak @andreasn @martinpitt @mvollmer