Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up[Enhancement] Introduce Link Target in Button Block #10128
Conversation
This comment has been minimized.
This comment has been minimized.
|
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: New version: |
This comment has been minimized.
This comment has been minimized.
|
Thank you for the heads-up @mkaz 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 |
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
Thank you very much for pointing that out @aduth |
This comment has been minimized.
This comment has been minimized.
|
@nfmohit-wpmudev Any chance you can refresh this before the 4.1 UI freeze tomorrow? |
This comment has been minimized.
This comment has been minimized.
|
@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 |
This comment has been minimized.
This comment has been minimized.
|
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 |
This comment has been minimized.
This comment has been minimized.
|
wouldn’t it be better if we created another |
youknowriad
requested a review
from WordPress/gutenberg-core
Jan 21, 2019
This comment has been minimized.
This comment has been minimized.
|
What's the status of this one? would be good to get this small improvement in. |
This comment has been minimized.
This comment has been minimized.
|
At some point it would be great to increase code reuse between the component which edits link here and in the link popup that This would make it easier to keep those two in sync. |
This comment has been minimized.
This comment has been minimized.
|
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 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: The styles around the URL input are quite difficult to untangle. |
youknowriad
added this to the 5.0 (Gutenberg) milestone
Jan 22, 2019
This comment has been minimized.
This comment has been minimized.
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. |
gziolo
added
[Block] Button
[Type] Enhancement
labels
Jan 22, 2019
This comment has been minimized.
This comment has been minimized.
|
See also #12738 for an option that is growing on me. |
This comment has been minimized.
This comment has been minimized.
|
Thank you for the insights guys, I'll check out |
gziolo
reviewed
Jan 31, 2019
|
Marking this PR as needs updates. Let us know when it's refreshed so we could review it |
This comment has been minimized.
This comment has been minimized.
|
Noting that updating to use |
This comment has been minimized.
This comment has been minimized.
|
PR refreshed. I've used |
gziolo
modified the milestones:
5.0 (Gutenberg),
5.1 (Gutenberg)
Feb 4, 2019
nfmohit-wpmudev
added some commits
Apr 19, 2019
This comment has been minimized.
This comment has been minimized.
@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:
That's a fair criticism, but somewhat subjective and not a regression.
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.
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.
Again this is a new feature, would love to see this addressed quickly though. |
This comment has been minimized.
This comment has been minimized.
|
@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'. |
This comment has been minimized.
This comment has been minimized.
@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?
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! |
This comment has been minimized.
This comment has been minimized.
@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:
I have no objections to merging this PR. I do have objections with what gets actually shipped in a Gutenberg release though |
This comment has been minimized.
This comment has been minimized.
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
approved these changes
Jul 10, 2019
|
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
merged commit a32d813
into
WordPress:master
Jul 10, 2019
1 check passed
github-actions
bot
added this to the Gutenberg 6.1 milestone
Jul 10, 2019
This comment has been minimized.
This comment has been minimized.
|
Thank you so much for all your help @talldan I look forward to joining on the left-over issues regarding this block so that we can make this perfect! Thank you again |
This comment has been minimized.
This comment has been minimized.
|
Follow up issues: (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:
|
This comment has been minimized.
This comment has been minimized.
|
Great job here @nfmohit-wpmudev |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
Thank you so much @youknowriad |
This comment has been minimized.
This comment has been minimized.
|
Thanks @nfmohit-wpmudev and @talldan ! I think all the pending items are covered in the new issues. |




nfmohit-wpmudev commentedSep 23, 2018
•
edited by gziolo
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:
Screenshots
Types of changes
This PR introduces a new
booleanattribute namedlinkTarget, which iftrue, applies the_blanktarget attribute. It addsToggleControlwithin the form element of the button block link input, which is displayed via a toggle-able ellipsisIconButton. The styles have been updated so that it looks similar to the link modal inRichText.Checklist: