-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Canvas][Layout Engine] Persistent grouping #25854
Changes from 21 commits
75370f2
2f3a51b
754a022
e7c90b3
9778e2e
d1e57e6
8815027
c8bda80
cf37642
c35088e
f427511
b8cf380
f0a3c97
b9de698
175edb4
d0dd92f
973e712
23b7bb5
a1ddd79
d0a77c8
20d575d
6ee1beb
ee0dad3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,7 +95,7 @@ const handleMouseDown = (commit, e, isEditable) => { | |
|
||
const keyCode = key => (key === 'Meta' ? 'MetaLeft' : 'Key' + key.toUpperCase()); | ||
|
||
const isNotTextInput = ({ tagName, type }) => { | ||
const isTextInput = ({ tagName, type }) => { | ||
// input types that aren't variations of text input | ||
const nonTextInputs = [ | ||
'button', | ||
|
@@ -111,11 +111,11 @@ const isNotTextInput = ({ tagName, type }) => { | |
|
||
switch (tagName.toLowerCase()) { | ||
case 'input': | ||
return nonTextInputs.includes(type); | ||
return !nonTextInputs.includes(type); | ||
case 'textarea': | ||
return false; | ||
default: | ||
return true; | ||
default: | ||
return false; | ||
} | ||
}; | ||
|
||
|
@@ -125,7 +125,7 @@ const handleKeyDown = (commit, e, isEditable, remove) => { | |
const { key, target } = e; | ||
|
||
if (isEditable) { | ||
if (isNotTextInput(target) && (key === 'Backspace' || key === 'Delete')) { | ||
if ((key === 'Backspace' || key === 'Delete') && !isTextInput(target)) { | ||
e.preventDefault(); | ||
remove(); | ||
} else if (!modifierKey(key)) { | ||
|
@@ -137,6 +137,16 @@ const handleKeyDown = (commit, e, isEditable, remove) => { | |
} | ||
}; | ||
|
||
const handleKeyPress = (commit, e, isEditable) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason you're not handling this in the
So pressing
Maybe that's intentional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it could technically work both ways (actually, keyPress will activate on the upward trajectory), it was simply a question of showing intent - one is concerned with signaling intent with a keypress gesture - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The difference would be if you wanted to include control, shift, option, etc. Keypress would ignore keys combined with option on a mac, I believe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, keypress does ignore those keys. My point was that |
||
const { key, target } = e; | ||
const upcaseKey = key && key.toUpperCase(); | ||
if (isEditable && !isTextInput(target) && 'GU'.indexOf(upcaseKey) !== -1) { | ||
commit('actionEvent', { | ||
event: upcaseKey === 'G' ? 'group' : 'ungroup', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to get into the habit of creating constants for strings like this. |
||
}); | ||
} | ||
}; | ||
|
||
const handleKeyUp = (commit, { key }, isEditable) => { | ||
if (isEditable && !modifierKey(key)) { | ||
commit('keyboardEvent', { | ||
|
@@ -150,6 +160,7 @@ export const withEventHandlers = withHandlers({ | |
onMouseDown: props => e => handleMouseDown(props.commit, e, props.isEditable), | ||
onMouseMove: props => e => handleMouseMove(props.commit, e, props.isEditable), | ||
onKeyDown: props => e => handleKeyDown(props.commit, e, props.isEditable, props.remove), | ||
onKeyPress: props => e => handleKeyPress(props.commit, e, props.isEditable), | ||
onKeyUp: props => e => handleKeyUp(props.commit, e, props.isEditable), | ||
onWheel: props => e => handleWheel(props.commit, e, props.isEditable), | ||
resetHandler: () => () => resetHandler(), | ||
|
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.
At some point, I'd like to see these become constants, perhaps in an enum in a separate module. I'd add a TODO, or an issue perhaps, for all follow-ups?