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

Move important admonition from the heading Options to Remarks #4566

Closed
Jwaegebaert opened this issue Feb 23, 2023 · 7 comments
Closed

Move important admonition from the heading Options to Remarks #4566

Jwaegebaert opened this issue Feb 23, 2023 · 7 comments
Assignees
Milestone

Comments

@Jwaegebaert
Copy link
Contributor

Jwaegebaert commented Feb 23, 2023

The idea is to move the important admonitions found under the heading Options to the Remarks heading. E.g. for the command spo cdn get there is an important admonition under the heading options talking about permissions which feels a bit out of place there.

Commands that need to be updated:

When you filter in the docs on access the tenant admin site you'll already find 52 commands where this change can apply.
A few example commands: spo cdn get, spo knowledgehub get, spo site appcatalog add


Originates from: #4521 (comment)

@Jwaegebaert Jwaegebaert added enhancement docs needs peer review Needs second pair of eyes to review the spec or PR labels Feb 23, 2023
@Jwaegebaert
Copy link
Contributor Author

Jwaegebaert commented Feb 23, 2023

@pnp/cli-for-microsoft-365-maintainers any thoughts regarding this suggestion?

@milanholemans
Copy link
Contributor

In my opinion the whole message with you can run this command if the user can access the tenant admin site is also very unclear to me, but let's leave it as is, we can't recheck all permissions for all these commands to make it more clear.

Let's definitely check which docs should be updated. We should probably build a script to check this because other admonitions with other messages can also be placed in a wrong position.
But looks good to me, let's open it up!

@milanholemans milanholemans added help wanted and removed needs peer review Needs second pair of eyes to review the spec or PR labels Feb 23, 2023
@waldekmastykarz
Copy link
Member

Does the remarks section show when we use the short help format? If it doesn't the moving this message means hiding it, which isn't a great idea. Perhaps it would be better to wait until we start working on adding permissions information to all commands and update it then ensuring that it will stay visible.

@milanholemans
Copy link
Contributor

Does the remarks section show when we use the short help format? If it doesn't the moving this message means hiding it, which isn't a great idea. Perhaps it would be better to wait until we start working on adding permissions information to all commands and update it then ensuring that it will stay visible.

We're putting these admonitions under Remarks for months now, only for old commands it's still under Options. So in fact we are already doing this now.

@Jwaegebaert
Copy link
Contributor Author

Does the remarks section show when we use the short help format? If it doesn't the moving this message means hiding it, which isn't a great idea. Perhaps it would be better to wait until we start working on adding permissions information to all commands and update it then ensuring that it will stay visible.

While I understand your suggestion to wait until we add permissions information to all commands, I believe that the remarks section would be a better location for those comments about the commands. Many of the important comments about commands can be found in this section.

Regarding the short help format, we also have a type that includes remarks. If the user wants to make sure they are fully informed about the usage of a command, it would be best practice to include this section in their help output.

@SmitaNachan
Copy link
Contributor

Can I work on it?

@Adam-it
Copy link
Contributor

Adam-it commented Feb 24, 2023

Can I work on it?

All yours 👍

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

Successfully merging a pull request may close this issue.

5 participants