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

Update LinkControl component to utilitse dynamic settings for additional settings "drawer" #18285

Merged
merged 10 commits into from
Nov 6, 2019

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 5, 2019

Closes #18206.

The new experimental Link UI provided by LinkControl allows for a currently selected link to have settings such as "Open in new tab"...etc

As things stand the only setting that is possible is "new tab" as this has been hard coded.

This PR updates so that multiple settings can be provided by passing the currentSettings prop to LinkControl:

<LinkControl
	// ...other props here
	currentSettings={ [
		{
			id: 'newTab',
			title: 'Open in New Tab',
			checked: false,
		},
		{ // this is custom!
			id: 'noFollow',
			title: 'No follow',
			checked: true,
		},
	] }
/>

How has this been tested?

Automated tests cover this change.

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected).

As this updates LinkControl we can expect this PR to break. It will need updating so that:

  • new-tab becomes newTab in any settings provided.

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.

@getdave getdave added the [Package] Block editor /packages/block-editor label Nov 5, 2019
@getdave getdave self-assigned this Nov 5, 2019
@getdave getdave added this to 👀 PRs to review in Navigation editor via automation Nov 5, 2019
@getdave getdave requested a review from obenland November 5, 2019 13:05
@getdave
Copy link
Contributor Author

getdave commented Nov 5, 2019

@phpbits This is relevant to #17557 (comment)

@getdave
Copy link
Contributor Author

getdave commented Nov 5, 2019

@retrofox I'm interested in how you see my change working with https://github.com/WordPress/gutenberg/pull/18062/files#diff-17e2993cb0b8cf64434015038b235162R214

Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

Testing

I've tested locally in my dev env. It looks pretty good!

link-control-dynamic

It's pretty easy to use and narrows very good with the block attributes:

<LinkControl
	currentSettings={ [
		{
			id: 'opensInNewTab',
			title: __( 'Open in New Tab' ),
			checked: opensInNewTab,
		},
		{
			id: 'noFollow',
			title: 'No follow',
			checked: noFollow,
		},
	] }
	onSettingsChange={ ( setting, value ) => setAttributes( { [ setting ]: value } ) }
/>

@retrofox
Copy link
Contributor

retrofox commented Nov 5, 2019

btw feel free to go-ahead not waiting for #18062 👍

@getdave
Copy link
Contributor Author

getdave commented Nov 6, 2019

I'll be rebasing this branch shortly and tweaking the visual styles. Then we're 👍 to merge.

Make settings an array so that it can be ordered. Fix incorrect conditional testing for .length to ensure it passes if settings _does_ have a length.
Previously only a “new tab” setting was hardcoded. Update so retain “new tab” as the default, but also allow for custom settings if/when provided.
@getdave getdave force-pushed the update/link-control-utilitse-dynamic-settings branch from ae9da61 to 0458a32 Compare November 6, 2019 11:56
@getdave
Copy link
Contributor Author

getdave commented Nov 6, 2019

@retrofox Having to pass the entire config object even to use the default setting feels like a lot of effort:

<LinkControl
	currentSettings={ [
		{
			id: 'opensInNewTab', // why can't I omit this?
			title: __( 'Open in New Tab' ), // why can't I omit this?
			checked: opensInNewTab, 
		},
	] }
	onSettingsChange={ ( setting, value ) => setAttributes( { [ setting ]: value } ) }

Is there a way to make this API nicer?

@retrofox
Copy link
Contributor

retrofox commented Nov 6, 2019

While I was testing I immediately thought that the id and checked properties could be combined in just one. At least it could be optional:
If the checked is not defined we can try to apply the value through of the id

<LinkControl
	currentSettings={ [
		{
			id: 'opensInNewTab',
			title: __( 'Open in New Tab' ), // why can't I omit this?
		},
	] }

About the title, maybe? I guess that maybe it could be handled by the parent component? Not sure, though. We can just remove the title if it isn't defined.

@retrofox
Copy link
Contributor

retrofox commented Nov 6, 2019

Dave, I think we should update how the Menu Item updates the link settings: something like that?

const updateLinkSetting = ( setter ) => ( setting, value ) => {
	setter( { [setting]: value } );
};

@retrofox
Copy link
Contributor

retrofox commented Nov 6, 2019

Also, we will need to update the setting ID here to opensInNewTab

@getdave
Copy link
Contributor Author

getdave commented Nov 6, 2019

Ok @retrofox this is updated.

@getdave getdave merged commit 3ab9fb6 into master Nov 6, 2019
Navigation editor automation moved this from 👀 PRs to review to ✅ Done Nov 6, 2019
@getdave getdave deleted the update/link-control-utilitse-dynamic-settings branch November 6, 2019 16:37
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Update LinkControl to handle multiple settings not only new tab
3 participants