Merged
Conversation
swissspidy
reviewed
Jan 28, 2020
9 tasks
barklund
commented
Jan 29, 2020
Contributor
|
@barklund There are a few minor console errors related to the PR that would be great to resolve before merging, too (e.g. missing |
Contributor
|
This ticket now covers - #6 |
| element.addEventListener( 'keydown', handleKeyPress ); | ||
|
|
||
| return () => { | ||
| if ( element ) { |
Contributor
There was a problem hiding this comment.
Why do we have to check if element exists here?
Contributor
Author
There was a problem hiding this comment.
I had some issues at some point with "element being undefined", but it appears they're not relevant anymore. Fixed in 6283157.
| // TODO Should be rewritten to use MouseTrap when added to . | ||
| const handleKeyPress = useCallback( ( evt ) => { | ||
| if ( [ KEY_UP, KEY_DOWN ].includes( evt.keyCode ) ) { | ||
| const direction = evt.keyCode === KEY_UP ? 1 : -1; |
Contributor
There was a problem hiding this comment.
I thought keyCode is deprecated via this comment.
Contributor
Author
There was a problem hiding this comment.
My bad, copy-paste from something else - also it felt weird. But didn't look more into it.
Merged
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Collaborator
|
@spacedmonkey Can you reply to the bot? 🙂 Thanks! |
Contributor
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Contributor
|
Hope you don't mind, but also tidied up the document inspector - f66fbd5 |
Contributor
Author
@spacedmonkey, I don't see any such console errors. Only an error about |
…ries-wp into feature/51-panel-refactor
Contributor
|
@barklund I get these errors when openning the browser After opening the documents inspector. After inserting a text element |
| border: 0 solid ${ ( { theme } ) => theme.colors.fg.v2 }; | ||
| border-top-width: ${ ( { isPrimary } ) => isPrimary ? 0 : '1px' }; | ||
| color: ${ ( { theme } ) => theme.colors.bg.v2 }; | ||
| padding: 10px 20px; |
Contributor
There was a problem hiding this comment.
Instead of putting the padding here, can we put it on the button here.
Contributor
Author
There was a problem hiding this comment.
This actually follows the W3 guideline on accordions 1-to-1. I have no opinion one way or the other - just followed their lead.
Contributor
Author
Those are also in master. @dvoytenko is fixing that in 43f91d5.
Fixed all of these in 46c4a62. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR includes some general changes to design and architecture for panels necessary for future work.
This PR will not address:
Partially fixes #51.
This is a duplicate of ampproject/amp-wp#4138, ported to this repo.