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

Social Links: Use URLInput #18905

Merged
merged 3 commits into from
Dec 5, 2019
Merged

Social Links: Use URLInput #18905

merged 3 commits into from
Dec 5, 2019

Conversation

jasmussen
Copy link
Contributor

Address feedback in #16897 (comment).

Something's missing, though, I'm getting Cannot read property 'value' of undefined. Could use advice here.

Address feedback in #16897 (comment).

Something's missing, though, I'm getting `Cannot read property 'value' of undefined`. Could use advice here.
@jasmussen jasmussen added the [Type] Code Quality Issues or PRs that relate to code quality label Dec 4, 2019
@jasmussen jasmussen requested review from mkaz and aduth December 4, 2019 08:17
@jasmussen jasmussen self-assigned this Dec 4, 2019
@aduth
Copy link
Member

aduth commented Dec 4, 2019

Thanks for jumping on this so quickly. Not immediately obvious to me why you'd be seeing that error. I'll have to take a closer look.

@aduth
Copy link
Member

aduth commented Dec 4, 2019

The main issue is URLInput's onChange returns the next value, not an event object.

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/url-input/README.md#onchange-url-string-post-object--function

diff --git a/packages/block-library/src/social-link/edit.js b/packages/block-library/src/social-link/edit.js
index b6d95585e..ea2a1d7c3 100644
--- a/packages/block-library/src/social-link/edit.js
+++ b/packages/block-library/src/social-link/edit.js
@@ -49,7 +49,7 @@ const SocialLinkEdit = ( { attributes, setAttributes, isSelected } ) => {
 						} } >
 						<URLInput
 							value={ url }
-							onChange={ ( event ) => setAttributes( { url: event.target.value } ) }
+							onChange={ ( nextURL ) => setAttributes( { url: nextURL } ) }
 							placeholder={ __( 'Enter Address' ) }
 						/>
 						<IconButton icon="editor-break" label={ __( 'Apply' ) } type="submit" />

I still see another error though, not entirely sure yet whether it's related to this branch.

@aduth
Copy link
Member

aduth commented Dec 4, 2019

Okay, the other error I was seeing seems to be caused by my LastPass browser extension. I assume it was getting confused that the text input was suddenly disappearing from the page after the updated URL is set.

I pushed up the previously-mentioned fix. I hope you don't mind, but I also lumped in a bit of verbiage tweaking to the CSS guidelines in hopes of clarifying why we target the common components.

Last thing I want to do here is just confirm there's not some behavior of URLInput we were trying to avoid by using a custom variation of it.

@jasmussen
Copy link
Contributor Author

Don't mind at all.

No, there's no custom stuff going on, only the tweaked placeholder text, and that appears to work fine. Thank you! I'll undraft this.

@jasmussen jasmussen marked this pull request as ready for review December 4, 2019 15:15
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

URLInput does do more stuff than what was previously supported here, mostly around suggestions. I guess maybe it was expected that suggested (local) URLs don't make much sense in the context of Social Media links. That could be true, but it seems like something we could make configurable on URLInput if it's really a problem.

In any case, this moves us in the right direction. 👍

@jasmussen
Copy link
Contributor Author

@mkaz can you give this PR a sanity check and merge if all is right?

Thanks again @aduth

@mkaz
Copy link
Member

mkaz commented Dec 5, 2019

This change looks good, 👍

Here is the original switch away from URLInput, unfortunately my commit doesn't include the why of the switch. I do remember the suggestions and placeholder being annoying.

This issue will probably help: #11852

@mkaz mkaz merged commit bcfeae2 into master Dec 5, 2019
@mkaz mkaz deleted the polish/social-links branch December 5, 2019 15:32
mkaz added a commit that referenced this pull request Dec 5, 2019
In #18905 Social Links was switched from a generic input field to using
URLInput, this change adds disableSuggetions to prevent the field to
suggest internal links or pages since Social Links are typically an
external link.
mkaz added a commit that referenced this pull request Dec 6, 2019
In #18905 Social Links was switched from a generic input field to using
URLInput, this change adds disableSuggetions to prevent the field to
suggest internal links or pages since Social Links are typically an
external link.
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants