-
Notifications
You must be signed in to change notification settings - Fork 687
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
Help window with supported commands and shortcuts #941
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, just a few points.
It would also be nice to be able to have a key-binding for this window as well. It's pretty common to just have "?" go to a keybindings/help window, but that wouldn't work if the user was in the text input. That also feels like something that can come later.
client/index.html
Outdated
</div> | ||
</div> | ||
|
||
<h2>Keyboard Shortcuts</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we stick the keyboard shortcuts at the top? It feels like that being a shorter section would make more sense to be at the top. Also, it's more likely to be the thing that people are initially wanting to look at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking, I switched both sections.
|
||
<h2>About The Lounge</h2> | ||
|
||
<p class="about"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, this feels a bit like it's being shoved in here. I'm not sure about this. It feels like a separate thing, to be honest. I know you want this to be an all encompassing help page, but this doesn't really feel like it fits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your concerns but here is my take on this:
- As this links to the release notes, the documentation and reporting a new issue, this seems more valuable to have it in the
Help
page than theSettings
page. Even without those links, this is more informative than configurable, so still a bit more appropriate. - I am going to add a bit of "help" value to it very soon as I'll be adding a couple links to new resources of the website.
Having it separate is reasonable but I think outside of the scope of this PR (and of the time I can reasonably spend on this project :D). One idea would be to have tabs in this page after we add similar tabs to the Settings page and About
would be one tab.
But again, timeframe-wise, we're not there yet, and I'd argue there are much more pressing things I'd like to spend my time on instead 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea would be to have tabs in this page after we add similar tabs to the Settings page and About would be one tab.
Oh goodness, please no. that really feels like overkill.
As this links to the release notes, the documentation and reporting a new issue, this seems more valuable to have it in the Help page than the Settings page.
Yeah, I guess so.
</div> | ||
</div> | ||
|
||
<div id="help" class="window"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it would be better to have this as a modal rather than a window? Not sure how you feel about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see where you're going with this: a "Quick Help" modal just like GitHub (hit ? outside an input
). While I see value in this because modals thrive when maintaining previous context on the view, I think this would become necessary if/when the help page becomes more comprehensive. So in other words, I think it's too early for this right now.
Other things to look at is that modals don't usually play well on mobile, and that we currently have no modals in the app to get inspiration from.
As mentioned in the PR description, my only goal with this PR is to move the actual content from the website to the client to move forward with documentation rework, and I cannot add another task to my current endless todolist :)
Are you OK keeping this in its separate window for now and improve it over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you OK keeping this in its separate window for now and improve it over time?
Yeah, fine
bd378c0
to
87b9a2a
Compare
Thanks for your review, @YaManicKill :)
Agreed, but I'll leave that to potential contributors who want to get involved in the project after we merge, so I can go back to my other focuses :) |
Pssst, dunno what you are talking about. To be honest, ctcp is so niche and specific that it's probably best to describe it as little as possible, and send to some more documentation about what ctcp actually is (the wikipedia page is pretty good, for example). Something like
or something like that. I don't think putting a longer explanation will actually help anyone. Also, just a quick thing, you have the command as
Should we change that to
|
87b9a2a
to
2be834b
Compare
Completely agree with all that, @YaManicKill. I made all these changes. Let me know if you have any other wording changes in mind. I think it's already a good start :)) |
@YaManicKill, ok with this after I applied your suggestions? :) Can I have 👍 from another @thelounge/maintainers too? Thanks! |
I will 👍 on the premise that we need documentation. But could you align the columns in tables, and I think left aligned text is better suited. |
@astorije small CSS changes: diff --git a/client/css/style.css b/client/css/style.css
index 81cbaab..9054cf0 100644
--- a/client/css/style.css
+++ b/client/css/style.css
@@ -1320,6 +1320,10 @@ kbd {
margin-top: .2em;
}
+#help .container {
+ max-width: 100%;
+}
+
#help .help-item {
display: table-row;
}
@@ -1333,7 +1337,7 @@ kbd {
#help .help-item .subject {
white-space: nowrap;
padding-right: 10px;
- text-align: right;
+ width: 200px;
}
#help .help-item .description p {
@@ -1815,6 +1819,15 @@ kbd {
#chat .title:before {
display: none;
}
+
+ #help .help-item .subject {
+ display: inline-block;
+ padding-bottom: 4px;
+ }
+
+ #help .help-item .description {
+ display: block;
+ }
}
@media (min-width: 769px) { |
This brings commands and keyboard shortcuts from the website, after a massive overhaul. It comes as part of the big documentation rewrite that I am currently doing. `kbd` design inspiration from GitHub, `code` design inspiration from Bootstrap. This help page is accessible from an icon in the sidebar, near the Settings icon.
2be834b
to
c1fc185
Compare
Hey @xPaw, thanks for this! Regarding the left alignment, I preferred aligned right, but that's very much personal preference + self-convinced when looking at how GitHub does it. I have set it left. Regarding the 200px width, I have done slightly differently using What do you think, still not good? I don't have a strong opinion. I do prefer this version, but I'm fine either way. That |
Help window with supported commands and shortcuts
This addresses #764, but it was not the starting point of this PR: I am currently rewriting the documentation (PR incoming) and the existing one only has Commands and Keyboard Shortcuts.
The website is not the best place for these and it makes more sense to have them available directly in the UI.
I also rewrote a lot of them to make them clearer and/or more up-to-date with the current state of the project.
My current and only goal for this PR is to move the 2 pages from the website to the UI, with correct description. There are many ways to go nuts with this: display shortcuts for the current platform only, hide the keyboard shortcuts on mobile, etc. This PR will not address any of these. I'll let future contributors help with that while I'm moving forward with the improvements on the documentation 😄
TODO
/ctcp
command. Any suggestion for this?Screenshots