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

Added missing static commands to documentation #11152

Closed
wants to merge 1 commit into from

Conversation

adamperkowski
Copy link
Contributor

Added missing commands (those without keymaps) to documentation.

Documented in Issue #10970.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

This file is auto-generated by cargo xtask docgen - it documents the typable commands (commands you can use in command mode :). The commands you added are static commands. They can't be used from command mode so they shouldn't have : in front of them. Instead they're meant to be bound to keys and they take no arguments. #10970 should be solved by adding a new document for static commands that is generated by cargo xtask docgen like this file

@adamperkowski
Copy link
Contributor Author

Working on it.

@adamperkowski
Copy link
Contributor Author

Did everything as it's supposed to be. Sorry for the inconvenience.

@adamperkowski
Copy link
Contributor Author

You think it would be better to create a separate doc page 2.4 for static commands? (i added a new section here)

book/src/commands.md Outdated Show resolved Hide resolved
xtask/src/docgen.rs Outdated Show resolved Hide resolved
xtask/src/docgen.rs Outdated Show resolved Hide resolved
xtask/src/docgen.rs Outdated Show resolved Hide resolved
@the-mikedavis
Copy link
Member

Yeah the page will be quite long with static and typable commands together. Let's use separate pages

@adamperkowski
Copy link
Contributor Author

Done.

@adamperkowski
Copy link
Contributor Author

Hi @the-mikedavis I just wanted to check in on the pull request. If you have any more feedback, please let me know. Whenever you have a moment, could you please take a look and consider merging? I would really appreciate it.

book/src/SUMMARY.md Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added the A-documentation Area: Documentation improvements label Jul 15, 2024
@the-mikedavis
Copy link
Member

Code-wise this looks good, just a note about the layout

@adamperkowski
Copy link
Contributor Author

Done.

the-mikedavis
the-mikedavis previously approved these changes Jul 15, 2024
@the-mikedavis
Copy link
Member

(The CI failure is from master, not related to these changes)

@adamperkowski adamperkowski changed the title Added missing commands (those without keymaps) to documentation Added missing static commands to documentation Jul 16, 2024
@adamperkowski
Copy link
Contributor Author

Hey, just checking on again. Will this PR be merged?

@pascalkuthe
Copy link
Member

Implementation looks good. I do wonder if we should emulate the command picker and also show default bindings here. While we already have a handwritten keymap docs it can be useful to see wether default bindings exist (and what they are) right away after finding a command here.One challenge with that is that bindings would be mode dependent I guess. I guess for now one putting the mode a command is mapped for in parenthesis could work. For example c-z (normal,select,insert),j (normal)

This would also cover some things missing from the keymaps docs (like c-z for suspend). I think often users can be familiar with a keybinding but may nit understand/know the bame/description (c-z is widely know, but the suspend command noch so much).

Thoughts @the-mikedavis

@adamperkowski
Copy link
Contributor Author

@pascalkuthe I could open another PR for that

@the-mikedavis
Copy link
Member

Hmm yeah that's a good idea. See the reverse_map function in keymap.rs and the command_palette implementation in commands.rs for examples of how to that display is working in the command palette (<space>?). It would be better to introduce that change in one go (i.e. this PR): any new PRs that add commands between merging this PR and a follow-up would cause conflicts and CI failures.

@adamperkowski adamperkowski marked this pull request as ready for review September 11, 2024 20:49
@adamperkowski
Copy link
Contributor Author

@pascalkuthe @the-mikedavis Done.

@adamperkowski
Copy link
Contributor Author

adamperkowski commented Sep 11, 2024

I noticed an issue with special characters like ` or <> in keybinds. It's messing up the markdown. Is this a thing with generating webpages too?
If that's the case, I really think not adding the keybinds is a better idea.

I could make docgen put a \ before them.

xtask/src/docgen.rs Outdated Show resolved Hide resolved
xtask/src/docgen.rs Show resolved Hide resolved
xtask/src/docgen.rs Outdated Show resolved Hide resolved
@adamperkowski
Copy link
Contributor Author

Added your suggestions, @the-mikedavis. I don't understand one thing though. Why are there duplicates? This one for example.

image

Commits squashed a98598f to f7daa47
@adamperkowski
Copy link
Contributor Author

Can't reopen the PR for some reason. I'll open a new one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Documentation improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants