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 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 requested review from retrofox, draganescu and talldan Nov 5, 2019
@getdave getdave requested review from ellatrix and youknowriad as code owners Nov 5, 2019
@getdave getdave self-assigned this Nov 5, 2019
@getdave getdave added this to 👀 PRs to review in Navigation block via automation Nov 5, 2019
@getdave getdave requested a review from obenland Nov 5, 2019
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 5, 2019

@phpbits This is relevant to #17557 (comment)

@getdave getdave mentioned this pull request Nov 5, 2019
12 of 12 tasks complete
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 5, 2019

@getdave getdave requested review from frontdevde and marekhrabe Nov 5, 2019
Copy link
Contributor

retrofox left a comment

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

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 5, 2019

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

@getdave

This comment has been minimized.

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.

getdave added 5 commits Nov 5, 2019
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 Nov 6, 2019
@getdave

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 6, 2019

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

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Nov 6, 2019

Ok @retrofox this is updated.

@getdave getdave merged commit 3ab9fb6 into master Nov 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Nov 6, 2019
@getdave getdave deleted the update/link-control-utilitse-dynamic-settings branch Nov 6, 2019
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
…ional settings "drawer" (WordPress#18285)

* Adds basic coverage test for new tab setting

* Refactor ‘new-tab` to camelCase for consistency

* Fix bug with null render if settings has a length and convert to array

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.

* Updates to loop over supplied settings

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.

* Updates docs

* Updates Nav Item Block to utilise new setting format

* Updates spacing between individual setting controls

* Update docs for settings

* Updates setting key/id to match attribute to simply and improve consistency

* Fix Nav Menu Block dynamic setting of attribute

Addresses WordPress#18285 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.