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

Add "open in new tab" feature to Social Links Block #25468

Merged
merged 6 commits into from
Sep 19, 2020

Conversation

fabiankaegy
Copy link
Member

@fabiankaegy fabiankaegy commented Sep 18, 2020

Description

I have modified the SocialLink and SocialLinks Blocks to include the ability to open in new tab. This was implemented using the contextProvider API to pass the setting down to the children.

It fixes the issue outlined in #20707

Closes #20707

How has this been tested?

This has been tested locally with by setting up a SocialLinks block and then updating it to make sure there are no deprecation errors.

Types of changes

New feature & Bug fix

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 updated all React Native files affected by any refactorings/renamings in this PR.

@fabiankaegy fabiankaegy requested a review from mkaz as a code owner September 18, 2020 20:34
@fabiankaegy fabiankaegy linked an issue Sep 18, 2020 that may be closed by this pull request
@fabiankaegy fabiankaegy added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Sep 18, 2020
@fabiankaegy fabiankaegy changed the title add target and rel setting in SocialLink Block Add "open in new tab" feature to Social Links Block Sep 18, 2020
@github-actions
Copy link

github-actions bot commented Sep 18, 2020

Size Change: +99 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-library/index.js 135 kB +99 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.33 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 8.41 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 128 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.59 kB 0 B
build/block-library/editor.css 8.59 kB 0 B
build/block-library/style-rtl.css 7.6 kB 0 B
build/block-library/style.css 7.59 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.77 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.5 kB 0 B
build/components/index.js 202 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.4 kB 0 B
build/compose/index.js 9.42 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 1.27 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.44 kB 0 B
build/edit-navigation/index.js 10.4 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.24 kB 0 B
build/edit-post/style.css 6.22 kB 0 B
build/edit-site/index.js 19.6 kB 0 B
build/edit-site/style-rtl.css 3.3 kB 0 B
build/edit-site/style.css 3.3 kB 0 B
build/edit-widgets/index.js 17 kB 0 B
build/edit-widgets/style-rtl.css 2.79 kB 0 B
build/edit-widgets/style.css 2.79 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.8 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.7 kB 0 B
build/server-side-render/index.js 2.61 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.74 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@fabiankaegy
Copy link
Member Author

@mkaz I just saw this ticket was a duplicate and saw your reply in one of the older PR's #20740 (comment) I think with that we can close this aswell

@mkaz
Copy link
Member

mkaz commented Sep 18, 2020

I was just reviewing this one and I'm not sure if the requiring only setting once is still worth holding out for, it is definitely the ideal scenario but something might be better than nothing. I'm not sure the efforts of allowing a parent block to pass down attributes to InnerBlock children.

@mkaz mkaz added [Block] Social Affects the Social Block - used to display Social Media accounts Needs Design Feedback Needs general design feedback. labels Sep 18, 2020
Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

I've tested this out and it works nicely.

@jasmussen we've had this before and overall, is it worth holding off the feature since it requires setting "open in new tab" on each link, or should we deploy. Ideal scenario is set once on parent and the children inherit.

@fabiankaegy
Copy link
Member Author

@mkaz I took a stab at using the contextProvider option and updated this PR to include that implementation. We can always go back to the earlier version though it you prefer that :)

@fabiankaegy
Copy link
Member Author

fabiankaegy commented Sep 18, 2020

This implementation currently really only allows you to toggle Open in new Tab on or of. I have not added the text field to modify the rel attribute directly like it is done in the button or how I mimicked it in the earlier implementation. But that would be a quick add if you think it would be beneficial.

@fabiankaegy fabiankaegy requested a review from mkaz September 18, 2020 21:39
@fabiankaegy fabiankaegy self-assigned this Sep 18, 2020
@mkaz
Copy link
Member

mkaz commented Sep 18, 2020

Much better, I hadn't used the context yet. I had earmarked the ticket for when it got complete, but hadn't circled back. I've got a minor change, but I think this is what we want 👍

I'll let Joen take a look and make sure he's happy with the change.

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

Couple of minor changes.

Thanks again!

packages/block-library/src/social-links/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/social-links/edit.js Outdated Show resolved Hide resolved
fabiankaegy and others added 2 commits September 18, 2020 23:59
Co-authored-by: Marcus Kazmierczak <marcus@mkaz.com>
Co-authored-by: Marcus Kazmierczak <marcus@mkaz.com>
@ZebulanStanphill ZebulanStanphill added the [Type] Enhancement A suggestion for improvement. label Sep 18, 2020
@jasmussen
Copy link
Contributor

This is what I see:

Screenshot 2020-09-19 at 10 05 12

Screenshot 2020-09-19 at 10 05 29

Superbly awesome, works perfectly. Open in new tab should be a global toggle for all links inside, and off by default. This PR does that beautifully. If Marcus digs the code, ship this as fast as you can. Thank you!

Copy link
Member

@mkaz mkaz left a comment

Choose a reason for hiding this comment

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

👍

@fabiankaegy fabiankaegy merged commit 8164fb0 into master Sep 19, 2020
@fabiankaegy fabiankaegy deleted the feature/25455-add-option-open-in-new-tab branch September 19, 2020 13:52
@github-actions github-actions bot added this to the Gutenberg 9.1 milestone Sep 19, 2020
@afercia
Copy link
Contributor

afercia commented Sep 24, 2020

On a post that already contained a Social Icons block with 6 links, I'm getting 6 PHP notices:

Notice: Undefined index: openInNewTab in /srv/www/wordpress-develop/public_html/build/wp-content/plugins/gutenberg/build/block-library/blocks/social-link.php on line 18

Screenshot 2020-09-24 at 14 39 34

@fabiankaegy @mkaz can you please have a look, when you have a chance?

@fabiankaegy
Copy link
Member Author

@afercia That is odd I had that in my checks. Will go over it again and have a look. My bad. Thank you for finding this

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 [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need add "open in new tab" feature in social share in Gutenberg Social icons Block: Open link in a new tab.
5 participants