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

[Enhancement] Introduce Link Target in Button Block #10128

Merged
merged 12 commits into from Jul 10, 2019

Conversation

@nfmohit-wpmudev
Copy link
Member

commented Sep 23, 2018

Description

Similar PR: #12738

This PR closes #8000 which requests the addition of an option to open the button link in a new tab. This also updates the design of the link form to make it look similar to the RichText link modal.

How has this been tested?

This PR has been tested by going through the following steps:

  1. Started a new post using the Gutenberg editor.
  2. Added the "Button" block.
  3. Made sure an ellipsis icon shows up within the link input of the button block, clicking which displays the "Open in New Window" option.
  4. Made sure the link target functionality works in the front-end.

Screenshots

pull-8000

Types of changes

This PR introduces a new boolean attribute named linkTarget, which if true, applies the _blank target attribute. It adds ToggleControl within the form element of the button block link input, which is displayed via a toggle-able ellipsis IconButton. The styles have been updated so that it looks similar to the link modal in RichText.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@mkaz

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

Nice fix, good consistency for URL input. It all works fine for me.

One thing, the layout for me doesn't look the same, but this is also the same prior to your change, so unlikely something you introduced. The enter and menu ellipsis show on the next line, not all in a single line. I'm using Firefox on Linux if that makes a difference.

Old version:

old-version

New version:

button-screenshot

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

Thank you for the heads-up @mkaz ❤️ And I'm sorry about the delay in response.

Thanks for testing the PR out. I believe the layout issue should be addressed in a different issue as it was already there previously. I'll try to check this out and send a PR for it as well. Thank you ❤️

@aduth

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

Rather than continue the separate implementation of URL input in buttons, had you considered the recommendation in #8000 to consolidate to introduce the target option by using the same URLInput as in other blocks? This will simplify long-term maintenance.

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2018

Thank you very much for pointing that out @aduth ❤️ I'll take another look into this and try to go forward using URLInput instead for this, as soon as possible.

@chrisvanpatten

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@nfmohit-wpmudev Any chance you can refresh this before the 4.1 UI freeze tomorrow? 😬

@chrisvanpatten chrisvanpatten referenced this pull request Oct 16, 2018
4 of 4 tasks complete
@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

@chrisvanpatten I'll try my best, but I'm a bit confused about the refresh so I think this will need to survive another round 😞

Speaking of being confused, it'd be really helpful if I can have some insights about this from @aduth 🙏 if possible. So, I revisited the other blocks which have a URL Input (e.g. the Paragraph block) and found that they are all using the RichText component. I'm extremely sorry if I sound unwitty but are we actually looking forward to implementing the RichText component in the Button block as well? If that is the case, how do you suggest I proceed about this, considering that the entire editing flow for the button block would change in that case? Thank you so very much ❤️

@chrisvanpatten

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

I can't speak for everyone but this feels like a case where we should get this version in, so it's in before UI freeze, and then iterate to use URLInput after that. But that's just a feeling — I'll let others comment with firm decisions :)

@bfintal

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

wouldn’t it be better if we created another URLInput that standardizes and implements a new tab toggle? URLInput seems to be usable for inlined links, the new one would be for non-inlined links such as buttons. A lot of customized blocks would benefit from this as well.

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Jan 21, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

What's the status of this one? would be good to get this small improvement in.

@gziolo

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

At some point it would be great to increase code reuse between the component which edits link here and in the link popup that RichText uses:
https://github.com/WordPress/gutenberg/blob/master/packages/format-library/src/link/inline.js#L253-L283

This would make it easier to keep those two in sync.

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

This PR also seems to address #6392, which proposes changing the Button block to use the same popover as is used elsewhere.

@gziolo There is a URLPopover component now, which makes it a bit easier, and should reduce the need to re-implement quite a few of the styles:
https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/url-popover/README.md

I think it's a good idea to refactor this PR to use it.

I did start refactoring the other components into presentational components (so there's URLPopover, URLPopoverForm, URLPopoverInput, URLPopoverSubmitButton) a while ago, but didn't quite make it into a PR:
https://github.com/WordPress/gutenberg/compare/refactor/link-interface-into-presentational-components?expand=1

The styles around the URL input are quite difficult to untangle. 😬

@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Jan 22, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

This PR also seems to address #6392, which proposes changing the Button block to use the same popover as is used elsewhere.

So there is another issue for that. Yes, that was my point. We should unify those two. It's a recurring issue people complain about.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

See also #12738 for an option that is growing on me.

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2019

Thank you for the insights guys, I'll check out URLPopover and try to refresh this PR with it.

@designsimply designsimply referenced this pull request Jan 29, 2019
4 of 4 tasks complete
@gziolo
Copy link
Member

left a comment

Marking this PR as needs updates. Let us know when it's refreshed so we could review it 👍

@designsimply

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

Noting that updating to use URLPopover should also solve #6841.

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2019

PR refreshed. I've used URLPopover and also included a panel in the InspectorControls to set a string rel value. Highly inspired by the updated image block. Thank you @gziolo for the review and thanks to everyone else for the insights.

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

as long as the pending issues get addressed before this feature gets released in a new version of the plugin 🙂

@afercia - shipping something that quickly, or patching a release generally only happens if there's a regression. Does this PR contain any regressions?

Of the four things you mentioned:

  • reconsider the "Open in New Tab" placement: is a bit buried in the Sidebar and inconsistent with other UIs

That's a fair criticism, but somewhat subjective and not a regression.

  • improve UI clarity: it's unclear if the link is automatically set or if there's the need of any user action (lack of "Apply" button)

The current block has an apply button, but it doesn't actually do anything. As mentioned above, the submit behaviour of the form has been overridden. I consider it a bad experience (and a bug) in the current implementation that a URL can be entered and applied without the user having clicked the button. The behaviour in this PR is actually the same as the current button block, but it removes the confusing button.

  • URL validation

I agree this would be great to add, but since it's not present on the current version of the button block it's a new feature rather than a regression.

  • spoken message

Again this is a new feature, would love to see this addressed quickly though.

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

@talldan thanks for the detailed recap. I'd like to remind everyone that any new code released in WordPress must be WCAG 2.0 compliant, according to the WordPress accessibility coding standards. Lack of user input validation and lack of proper feedback after an user action are specific SCAG violations, regardless of whether they're regressions 🙂

I'd really like to avoid to ship new code that's not accessible as it should be. It happened a bit too much often in the past and then it took months to be "improved'.

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

I'd really like to avoid to ship new code that's not accessible as it should be. It happened a bit too much often in the past and then it took months to be "improved'.

@afercia It is really frustrating when that happens, and it can be hard to get a fix merged, I've seen that with a few of my PRs.

At the same time, we've identified that these accessibility issues have been present in the button block from launch. When I search on github for any of the accessibility issues related to the button block that you've mentioned there are none, and that explains why they haven't been fixed.

If we do want to get those things fixed, my belief is that it'd be beneficial to ship this first, and then tackle those issues individually. Why?

  1. Multiple devs could pick up the issues, rather than it being left to the one contributor
  2. Smaller individual PRs are easier to merge and ship. Adding those improvements to this branch (which already has a massive history) isn't the most productive approach.
  3. Less chances of merge conflicts with this PR. Other devs may even view this PR as a blocker and choose not to work on fixes while it's open.

I'm ready and willing to work on one or two of the issues mentioned. But I'm not going to delay this PR any further by requesting further changes. I also believe it's unfair to @nfmohit-wpmudev to block his PR for even longer by asking him to start fixing issues he hasn't even caused. He's already put so much work into it!

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

we've identified that these accessibility issues have been present in the button block from launch. When I search on github for any of the accessibility issues related to the button block that you've mentioned there are none, and that explains why they haven't been fixed.

@talldan I guess you haven't found specific accessibility GitHub issues because the accessibility feedback was given directly on the issues and PRs during the button block implementation, and never addressed, unfortunately.

As I commented above:

@talldan I have no objections to merging this PR, as long as the pending issues get addressed before this feature gets released in a new version of the plugin

I have no objections to merging this PR. I do have objections with what gets actually shipped in a Gutenberg release though 🙂 Iterations can happen at any time, but non-accessible code should not be released.

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

@talldan I guess you haven't found specific accessibility GitHub issues because the accessibility feedback was given directly on the issues and PRs during the button block implementation, and never addressed, unfortunately.

Oh no! Alright, lets make sure that doesn't happen this time. I'll merge this PR and then start making some issues in github so that those things are captured. No guarantees these things will be shipped in the next release, but as we're right at the start of the release cycle there's as good a chance as any. I'll also try to do my best to try to push those accessibility issues forwards and pick up at least one of the tasks.

@talldan
Copy link
Contributor

left a comment

This has been through quite a bit of iteration over the last ten months. Thanks for sticking with it @nfmohit-wpmudev, massive kudos!

I think it's time to merge it.

There are definitely some existing issues (accessibility and otherwise) with the button block that this PR has resurfaced. Let's try to address them in follow-ups. I'll get to working on some issues to cover them.

@nfmohit-wpmudev nfmohit-wpmudev merged commit a32d813 into WordPress:master Jul 10, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@github-actions github-actions bot added this to the Gutenberg 6.1 milestone Jul 10, 2019

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

Thank you so much for all your help @talldan ❤️
I'd credit this PR to you, I mostly just followed your instructions 😝

I look forward to joining on the left-over issues regarding this block so that we can make this perfect! Thank you again ❤️

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Follow up issues:

  • Link validation / audible messages (#16494)
  • Link UI consistency (#16495)

(I'll edit this to add more as I create them)

Also created a PR based on code review comment by @noisysocks, I've asked him to review it:

  • Add docs and improve naming of showSuggestionsOverride (#16497)
@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Great job here @nfmohit-wpmudev

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Me again @afercia 😄

I've created two issues here that I think cover your comments.

Do let me know if I missed anything and I'll address it by updating the issues or creating other issues.

I decided to create one issue, #16494 to cover the issue of link validation and screenreader feedback when adding a link, since I think they're related.

#16495 covers the issues mentioned about the UI consistency. I think this one should also consider whether the settings should be in the sidebar (if the UI were made more consistent they wouldn't be in the sidebar).

I had a scrollback to check for anything else. I think those two issues cover everything.

@nfmohit-wpmudev

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

Thank you so much @youknowriad ❤️

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

Thanks @nfmohit-wpmudev and @talldan ! I think all the pending items are covered in the new issues.

@nfmohit-wpmudev nfmohit-wpmudev deleted the nfmohit-wpmudev:update/button-block/8000 branch Jul 18, 2019

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

[Enhancement] Introduce Link Target in Button Block (WordPress#10128)
* Introduce link target in Button block

* Updated popover style

* Moved 'Open in New Tab' toggle to InspectorControls

* Implemented inline link replacing URLPopover

* Updated button styles

* Make the BaseContrrol available in the DOM always, but hide it with CSS until the block is selected

* Limit width of the text field if empty

* Fix width and positioning issues of the URLInput and Autocomplete suggestions list

* Prevent autocomplete suggestions list from showing up when block is deselected

* Fixed the rel attribute not working issue

* Fixed validation errors

* Reverted changes from the package-lock.json file

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

[Enhancement] Introduce Link Target in Button Block (WordPress#10128)
* Introduce link target in Button block

* Updated popover style

* Moved 'Open in New Tab' toggle to InspectorControls

* Implemented inline link replacing URLPopover

* Updated button styles

* Make the BaseContrrol available in the DOM always, but hide it with CSS until the block is selected

* Limit width of the text field if empty

* Fix width and positioning issues of the URLInput and Autocomplete suggestions list

* Prevent autocomplete suggestions list from showing up when block is deselected

* Fixed the rel attribute not working issue

* Fixed validation errors

* Reverted changes from the package-lock.json file
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.