-
Notifications
You must be signed in to change notification settings - Fork 4.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
Editor: Unify right click override preference #57468
Changes from all commits
f1ff43e
ff6bbd2
e0a10d5
f289da3
eb43d98
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 |
---|---|---|
@@ -1,26 +1,31 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { compose } from '@wordpress/compose'; | ||
import { withSelect, withDispatch } from '@wordpress/data'; | ||
import { useSelect, useDispatch } from '@wordpress/data'; | ||
import { ___unstablePreferencesModalBaseOption as BaseOption } from '@wordpress/interface'; | ||
import { store as preferencesStore } from '@wordpress/preferences'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { store as editPostStore } from '../../../store'; | ||
|
||
export default compose( | ||
withSelect( ( select, { featureName } ) => { | ||
const { isFeatureActive } = select( editPostStore ); | ||
return { | ||
isChecked: isFeatureActive( featureName ), | ||
}; | ||
} ), | ||
withDispatch( ( dispatch, { featureName, onToggle = () => {} } ) => ( { | ||
onChange: () => { | ||
onToggle(); | ||
dispatch( editPostStore ).toggleFeature( featureName ); | ||
}, | ||
} ) ) | ||
)( BaseOption ); | ||
export default function EnableFeature( props ) { | ||
const { | ||
scope = 'core/edit-post', | ||
featureName, | ||
onToggle = () => {}, | ||
...remainingProps | ||
} = props; | ||
const isChecked = useSelect( | ||
( select ) => !! select( preferencesStore ).get( scope, featureName ), | ||
[ scope, featureName ] | ||
); | ||
const { toggle } = useDispatch( preferencesStore ); | ||
const onChange = () => { | ||
onToggle(); | ||
toggle( scope, featureName ); | ||
}; | ||
return ( | ||
<BaseOption | ||
onChange={ onChange } | ||
isChecked={ isChecked } | ||
{ ...remainingProps } | ||
/> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
|
||
export default function convertEditorSettings( data ) { | ||
let newData = data; | ||
const settingsToMoveToCore = [ 'allowRightClickOverrides' ]; | ||
|
||
settingsToMoveToCore.forEach( ( setting ) => { | ||
if ( data?.[ 'core/edit-post' ]?.[ setting ] !== undefined ) { | ||
newData = { | ||
...newData, | ||
core: { | ||
...newData?.core, | ||
[ setting ]: data[ 'core/edit-post' ][ setting ], | ||
}, | ||
}; | ||
delete newData[ 'core/edit-post' ][ setting ]; | ||
} | ||
|
||
if ( data?.[ 'core/edit-post' ]?.[ setting ] !== undefined ) { | ||
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. Should it be 'core/edit-site' here? 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. Though potentially the preference for the site editor can be deleted at the same time as the data for the post editor (after line 18) given it's being disregarded and no longer used for anything. 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. oops, right, I'll fix it in one of the other PRs 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. It can't be on line 18 though because it might exist while the post editor one won't. 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. Fix included here 429a995 |
||
delete newData[ 'core/edit-site' ][ setting ]; | ||
} | ||
} ); | ||
|
||
if ( Object.keys( newData?.[ 'core/edit-post' ] ?? {} )?.length === 0 ) { | ||
delete newData[ 'core/edit-post' ]; | ||
} | ||
|
||
if ( Object.keys( newData?.[ 'core/edit-site' ] ?? {} )?.length === 0 ) { | ||
delete newData[ 'core/edit-site' ]; | ||
} | ||
|
||
return newData; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
/** | ||
* Internal dependencies | ||
*/ | ||
import convertEditorSettings from '../convert-editor-settings'; | ||
|
||
describe( 'convertEditorSettings', () => { | ||
it( 'converts the `allowRightClickOverrides` property', () => { | ||
const input = { | ||
'core/edit-post': { | ||
allowRightClickOverrides: false, | ||
}, | ||
}; | ||
|
||
const expectedOutput = { | ||
core: { | ||
allowRightClickOverrides: false, | ||
}, | ||
}; | ||
|
||
expect( convertEditorSettings( input ) ).toEqual( expectedOutput ); | ||
} ); | ||
} ); |
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.
Should we do the same for
core/edit-site
store? Maybe first check if it's already set incore
scope..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.
absence of "core/edit-post" value doesn't really mean we don't have a preference, it just means that it was not explicitly set. So I think we should just keep it simple as is.