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

right-sidebar: Add shortcut to keyboard instructions in the bottom. #9182

Closed
wants to merge 1 commit into from

Conversation

shubham-padia
Copy link
Member

Fixes #8605.
Adds a link to the keyboard shortcuts popup at the bottom of the right
sidebar.

Current Version:
selection_090

Alternative versions:

1.) Keyboard-shortcuts in black colour instead of the gray color of sidebar-title class.
selection_089

2.) only keyboard icon
selection_091

3.) All user help links:
selection_088

Testing Plan:
Manual testing

@zulipbot
Copy link
Member

Hello @zulip/server-sidebars members, this pull request was labeled with the "area: right-sidebar" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

I think I'd prefer that this sit always in the bottom-right corner, rather than right after "Group PMs". And I think just having the icon in the corner is probably right (no need for the label).

@showell
Copy link
Contributor

showell commented Apr 23, 2018

I'll second the idea that a small icon (ideally looking like "?") is probably sufficient, with hover text to explain what it is.

@timabbott
Copy link
Sponsor Member

I think an icon that's a keyboard is the right call; Dropbox Paper does that and it looks pretty nice.

shubham-padia added a commit to shubham-padia/zulip that referenced this pull request Apr 25, 2018
…ner.

Fixes zulip#9182.
Adds a link to the keyboard shortcuts popup at the bottom-right corner
of the right sidebar. A div containing whitespace is added above the
keyboard icon to keep it at the bottom-right corner of the sidebar.
The height of the whitespace is calculated in `resize.js`. A tooltip
saying `Keyboard Shortcuts` has been added to the icon.
@zulipbot zulipbot added size: M and removed size: S labels Apr 25, 2018
shubham-padia added a commit to shubham-padia/zulip that referenced this pull request Apr 25, 2018
…ner.

Fixes zulip#9182.
Adds a link to the keyboard shortcuts popup at the bottom-right corner
of the right sidebar. A div containing whitespace is added above the
keyboard icon to keep it at the bottom-right corner of the sidebar.
The height of the whitespace is calculated in `resize.js`. A tooltip
saying `Keyboard Shortcuts` has been added to the icon.
@shubham-padia
Copy link
Member Author

@timabbott I've made the required changes, the failing tests are due to some other commits I think.
key_cropped_720p
Following are some screenshots of the complete browser window for diff. manual testing cases which've been tried out:
720p: link
Full user list 1080p: link
Incomplete user list 1080p: link

shubham-padia added a commit to shubham-padia/zulip that referenced this pull request Apr 25, 2018
…ner.

Fixes zulip#9182.
Adds a link to the keyboard shortcuts popup at the bottom-right corner
of the right sidebar. A div containing whitespace is added above the
keyboard icon to keep it at the bottom-right corner of the sidebar.
The height of the whitespace is calculated in `resize.js`. A tooltip
saying `Keyboard Shortcuts` has been added to the icon.
@timabbott
Copy link
Sponsor Member

@shubham-padia this looks great, but we should do a few tweaks:

  • The label should be "Keyboard shortcuts (?)" to show the keyboard shortcut for it; the (?) should be in a slightly lighter font. See the CSS for the actions menu popover for an example of how we do this.
  • We should have it be vertically centered within that bottom row
  • It should be 25-50% bigger so that it feels sizable within that row.

@timabbott
Copy link
Sponsor Member

@shubham-padia it looks like you pushed a new version after my last comment, but then didn't post a comment on GitHub. It turns out, GitHub doesn't send a notification to anyone else when you do that. So please always comment when you update a PR and are ready for another review.

@shubham-padia
Copy link
Member Author

@timabbott I don't remember pushing a new version after your last comment. The changes requested are yet to be made. The conflicts are due to the commits made into the zulip master branch I think.

shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 7, 2018
…ner.

Fixes zulip#9182.
Adds a link to the keyboard shortcuts popup at the bottom-right corner
of the right sidebar. A div containing whitespace is added above the
keyboard icon to keep it at the bottom-right corner of the sidebar.
The height of the whitespace is calculated in `resize.js`. A tooltip
saying `Keyboard Shortcuts` has been added to the icon.
shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 7, 2018
…ner.

Fixes zulip#9182.
Adds a link to the keyboard shortcuts popup at the bottom-right corner
of the right sidebar. A div containing whitespace is added above the
keyboard icon to keep it at the bottom-right corner of the sidebar.
The height of the whitespace is calculated in `resize.js`. A tooltip
saying `Keyboard Shortcuts` has been added to the icon.
@shubham-padia
Copy link
Member Author

@timabbott I've made the requested changes. I (unsuccessfully) tried to put the keyboard shortcut in the compose-container div for better vertical alignment, but it didn't work out, so got back to the current approach again.
screenshot from 2018-05-08 00-13-26

shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 7, 2018
…ner.

Fixes zulip#9182.
Adds a link to the keyboard shortcuts popup at the bottom-right corner
of the right sidebar. A div containing whitespace is added above the
keyboard icon to keep it at the bottom-right corner of the sidebar.
The height of the whitespace is calculated in `resize.js`. A tooltip
saying `Keyboard Shortcuts(?)` has been added to the icon.
@timabbott
Copy link
Sponsor Member

This looks great, visually. Do we really need to do set its height using logic in resize.js? I would have thought we could just do something with pure CSS for positioning this.

(The main downside of the current approach is that the thing jumps a bit every time I reload the page)

@synicalsyntax do you have any ideas?

shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 7, 2018
…ner.

Fixes zulip#9182. Adds a link to the keyboard shortcuts popup at the
bottom-right corner of the right sidebar. A tooltip saying
`Keyboard Shortcuts(?)` has been added to the icon. The icon is
positioned using `position: fixed`.
@zulipbot zulipbot added size: S and removed size: M labels May 7, 2018
shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 7, 2018
…ner.

Fixes zulip#9182. Adds a link to the keyboard shortcuts popup at the
bottom-right corner of the right sidebar. A tooltip saying
`Keyboard Shortcuts(?)` has been added to the icon. The icon is
positioned using `position: fixed`.
@shubham-padia
Copy link
Member Author

@timabbott I've pushed some new changes removing the white space logic.

shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 7, 2018
…ner.

Fixes zulip#9182. Adds a link to the keyboard shortcuts popup at the
bottom-right corner of the right sidebar. A tooltip saying
`Keyboard Shortcuts(?)` has been added to the icon. The icon is
positioned using `position: fixed`.
shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 16, 2018
…ner.

Fixes zulip#9182.
Adds a link to the keyboard shortcuts popup at the bottom-right corner
of the right sidebar. A div containing whitespace is added above the
keyboard icon to keep it at the bottom-right corner of the sidebar.
The height of the whitespace is calculated in `resize.js`. A tooltip
saying `Keyboard Shortcuts(?)` has been added to the icon.
@zulipbot zulipbot added size: M and removed size: S labels May 16, 2018
@shubham-padia
Copy link
Member Author

shubham-padia commented May 16, 2018

@timabbott FYI this is open for review.

…ner.

Fixes zulip#9182. Adds a link to the keyboard shortcuts popup at the
bottom-right corner of the right sidebar. A tooltip saying
`Keyboard Shortcuts(?)` has been added to the icon. The icon is
positioned using `position: fixed`.
@timabbott
Copy link
Sponsor Member

Nice. Merged, thanks @shubham-padia!

@timabbott timabbott closed this May 16, 2018
timabbott pushed a commit that referenced this pull request May 16, 2018
…ner.

Fixes #9182. Adds a link to the keyboard shortcuts popup at the
bottom-right corner of the right sidebar. A tooltip saying
`Keyboard Shortcuts(?)` has been added to the icon. The icon is
positioned using `position: fixed`.
shubham-padia added a commit to shubham-padia/zulip that referenced this pull request May 27, 2018
…ner.

Fixes zulip#9182. Adds a link to the keyboard shortcuts popup at the
bottom-right corner of the right sidebar. A tooltip saying
`Keyboard Shortcuts(?)` has been added to the icon. The icon is
positioned using `position: fixed`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants