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

Modifies shortcut hierarchy in modal #16724

Merged
merged 5 commits into from
Aug 8, 2019
Merged

Modifies shortcut hierarchy in modal #16724

merged 5 commits into from
Aug 8, 2019

Conversation

mapk
Copy link
Contributor

@mapk mapk commented Jul 23, 2019

Fixes #16691. Moves the shortcut to open shortcut modal to its own section right under the modal header.

How has this been tested?

Tested locally.

Screenshots

Before:

Screen Shot 2019-07-23 at 12 02 03 PM

Proposed:

Screen Shot 2019-07-23 at 11 57 03 AM

Types of changes

CSS changes along with a bit of Javascript in the keyboard-shortcut config file.
Non-breaking change.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@mapk mapk added Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility labels Jul 23, 2019
@mapk mapk self-assigned this Jul 23, 2019
@talldan
Copy link
Contributor

talldan commented Jul 24, 2019

@mapk I hope you don't mind, I pushed a few commits to tidy up a few things:

  • First commit - even though a title wasn't being specified for mainShortcut shortcuts, an empty <h2> element was still being inserted into the DOM. This commit makes sure the <h2> isn't inserted when a title isn't specified.
  • Second commit - adds a class name to the group of shortcuts so that nth-child doesn't have to be used.
  • Third commit - update the snapshot tests.

@mapk
Copy link
Contributor Author

mapk commented Jul 24, 2019

@talldan I love the help! Thanks. I'm browsing your commits to brush up my skills!

jorgefilipecosta
jorgefilipecosta approved these changes Jul 26, 2019
View reviewed changes
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I left a comment regarding the usage of a fixed number suggesting a variable instead. Other than that the changes look good to me 👍

packages/edit-post/src/components/keyboard-shortcut-help-modal/style.scss Outdated
&__section {
margin: 0 0 2rem 0;
}

&__main-shortcuts .edit-post-keyboard-shortcut-help__shortcut-list {
// Push the shortcut to be flush with top modal header.
margin-top: -17px;
Copy link
Member

Choose a reason for hiding this comment

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

Can we use margin-top: -$panel-paddingwhich is -16px to avoid an hardcoded value?

@mapk
Copy link
Contributor Author

mapk commented Jul 31, 2019

@jorgefilipecosta I couldn't just use the -$panel-padding due to a top border still being exposed.

Screen Shot 2019-07-31 at 3 10 24 PM

So I opted to add a -$border-width in addition to -$panel-padding in as well. It appears to be looking good now.

Screen Shot 2019-07-31 at 3 29 22 PM

@enriquesanchez
Copy link
Contributor

I think this is a small but nice addition. It should bring more awareness of how to invoke keyboard shortcuts modal.

@enriquesanchez enriquesanchez removed Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. labels Aug 7, 2019
@mapk
Copy link
Contributor Author

mapk commented Aug 7, 2019

Thanks @enriquesanchez!

@jorgefilipecosta can I get one last code review. I'd like to have a sanity check on using two variables to solve this.

@talldan
Copy link
Contributor

talldan commented Aug 8, 2019

I'm not entirely convinced about using $panel-padding as none of the rest of the styles in this stylesheet use those variables.

In testing though, this is working well. Let's get it in.

@talldan talldan added [Type] Enhancement A suggestion for improvement. General Interface Parts of the UI which don't fall neatly under other labels. labels Aug 8, 2019
@talldan talldan merged commit 5e79106 into master Aug 8, 2019
@talldan talldan deleted the update/keyboard-shortcut-hierarchy branch August 8, 2019 09:42
@mapk
Copy link
Contributor Author

mapk commented Aug 8, 2019

Thanks, @talldan. 🎉

@youknowriad youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
@mapk @gziolo
Modifies shortcut hierarchy in modal (#16724)
eca5584 
* Fixes #16691. Moves the shortcut to open shortcut modal to it's own section right under the modal header.

* Only render shortcut title when a value is provided

* Use classname to modify style for first shortcut

* Update snapshots

* Using variable for elements top-margin for positioning.
gziolo pushed a commit that referenced this pull request Aug 29, 2019
@mapk @gziolo
Modifies shortcut hierarchy in modal (#16724)
9f32f13 
* Fixes #16691. Moves the shortcut to open shortcut modal to it's own section right under the modal header.

* Only render shortcut title when a value is provided

* Use classname to modify style for first shortcut

* Update snapshots

* Using variable for elements top-margin for positioning.
This was referenced Aug 1, 2024
Open
Open
@WontonSam WontonSam mentioned this pull request Aug 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgefilipecosta jorgefilipecosta jorgefilipecosta approved these changes

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@mapk mapk

Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 6.3
Development

Successfully merging this pull request may close these issues.

Add keyboard shortcut to show keyboard shortcuts to top of list
5 participants
@mapk @talldan @enriquesanchez @jorgefilipecosta @youknowriad