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

Switch Social Links to use LinkControl component #20740

Closed
wants to merge 4 commits into from

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Mar 9, 2020

Description

This updates the Social Links to use the LinkControl component instead of URLInput, this allows for setting the "opens in external tab" property.

Fixes #20707

How has this been tested?

  • Create social link, make sure you can add URL
  • Toggle opens in new tab, confirm when saved it does what is expected
  • Confirm previous social links created prior to patch do not break

Screenshots

Screenshot from 2020-03-09 12-36-15

Types of changes

  • Switch URLInput to LinkControl
  • Adds new attribute opensInNewTab
  • Update fixtures

@mkaz mkaz added [Block] Social Affects the Social Block - used to display Social Media accounts [Type] Enhancement A suggestion for improvement. labels Mar 9, 2020
@github-actions
Copy link

github-actions bot commented Mar 9, 2020

Size Change: +117 B (0%)

Total Size: 864 kB

Filename Size Change
build/block-library/index.js 115 kB +117 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.6 kB 0 B
build/block-editor/style.css 10.5 kB 0 B
build/block-library/editor-rtl.css 7.36 kB 0 B
build/block-library/editor.css 7.36 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.75 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.64 kB 0 B
build/edit-post/style.css 8.64 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.44 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.97 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@jasmussen
Copy link
Contributor

Re-using UI components to create consistency is a noble goal, 👍 👍 on that aspect.

I don't personally think the "open in new tab" feature is something anyone should use, so good to default to off. Specifically in the case of social links, this feels like a property you set on the social links block itself, not each individual block — I have a hard time picturing why you'd want some of your social links to open in new tabs, and not others. But I imagine that would create a ton of extra complexity. So probably futile.

However I'm seeing a duplicate Enter sign:

Screenshot 2020-03-10 at 14 11 18

Which is a little weird, as it doesn't look like you've modified the control itself. Any idea what's up here?

Also, we'll want some different placeholder text than "Search or type url" (btw shouldn't url be capitalized?)

We could go with just "Enter address" which is what the old version said.

@mkaz
Copy link
Member Author

mkaz commented Mar 10, 2020

The placeholder text is from the LinkControl component, with that I realized it uses suggestions for local links and does not appear to have a disable flag like URLInput. In my tests I was typing URLs but if I start to type a local blog post title it shows.

Combined with the "open in tab" on every link, we might not be able to use the LinkControl component as-is.

Maybe just adding "opensInNewTab" attribute to the container SocialLinks block and then pass that down to the contained links would be the better approach, set once and apply to all.

@jasmussen jasmussen added the Needs Design Feedback Needs general design feedback. label Mar 10, 2020
@mkaz
Copy link
Member Author

mkaz commented Mar 11, 2020

I'm going to close this PR, because using the LinkControl component is not quite what we want to do because it would require each link to set the open in new tabs attribute — this is awkward.

The ideal solution is to add the property to the parent Social Links block, and then on server-side render read the property and attach the target attribute as needed. For this we need to wait on: #19685

@mkaz mkaz closed this Mar 11, 2020
@mkaz mkaz deleted the fix/20707-social-new-tab branch March 11, 2020 16:56
@mkaz mkaz removed the Needs Design Feedback Needs general design feedback. label Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Social icons Block: Open link in a new tab.
2 participants