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

Change the URL Input label to match Classic #9893

Merged
merged 4 commits into from Oct 4, 2018

Conversation

@pento
Member

pento commented Sep 14, 2018

Description

The current URL input label "Paste URL or type" doesn't actually say what typing does, and it hides the ability to search for posts/pages to link to.

Changing it to match the label in Classic, "Paste URL or type to search" tells the author what it is that typing will do, and makes the search functionality more discoverable.

In order to make the longer label fit, this change increases the width of the URL input box.

Screenshots

url input in Gutenberg

url input in classic

Checklist:

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

@pento pento added this to the 3.9 milestone Sep 14, 2018

@pento pento self-assigned this Sep 14, 2018

@pento pento requested a review from WordPress/gutenberg-core Sep 14, 2018

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 14, 2018

Contributor

The UrlLink button is always a tricky component to tweak.

I noticed some regressions on this PR

screen shot 2018-09-14 at 10 27 23

screen shot 2018-09-14 at 10 27 57

Also, I think it's probably better to keep the smaller size for mobile.

Contributor

youknowriad commented Sep 14, 2018

The UrlLink button is always a tricky component to tweak.

I noticed some regressions on this PR

screen shot 2018-09-14 at 10 27 23

screen shot 2018-09-14 at 10 27 57

Also, I think it's probably better to keep the smaller size for mobile.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 14, 2018

Contributor

I'm moving this PR to 4.0 feel free to merge though if it makes the 3.9 cut

Contributor

youknowriad commented Sep 14, 2018

I'm moving this PR to 4.0 feel free to merge though if it makes the 3.9 cut

@youknowriad youknowriad modified the milestones: 3.9, 4.0 Sep 14, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 30, 2018

Member

@youknowriad thank you for the feedback, @pento and I have fixed those visual bugs and restricted the width increases to non-mobile devices.

Member

jorgefilipecosta commented Sep 30, 2018

@youknowriad thank you for the feedback, @pento and I have fixed those visual bugs and restricted the width increases to non-mobile devices.

@talldan

Looks good to me from a code point of view, but I noticed this overlapping is quite pronounced now, so would prefer if a designer decided whether to approve the change:

screen shot 2018-10-04 at 12 27 15 pm

It does also happen in master, but not to the same degree. The popover does have some logic to stop it escaping the browser window's bounding box - maybe it's possible to change that so that it stays within the editor's bounding box instead.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 4, 2018

Member

Note that a submenu will enter the editor bounding box and cause the issue of a popover appearing over it too. 🤷‍♂️

Member

tofumatt commented Oct 4, 2018

Note that a submenu will enter the editor bounding box and cause the issue of a popover appearing over it too. 🤷‍♂️

@tofumatt tofumatt requested review from jasmussen and karmatosed Oct 4, 2018

@tofumatt

The overlap is already an issue that needs fixing and this is definite usability improvement.

I'd welcome design thoughts but they shouldn't be a blocker for this issue. I say 🚢 and we can improve the overlap in a follow-up issue.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 4, 2018

Contributor

Yep. If this works fine on mobile, then go for it.

Contributor

jasmussen commented Oct 4, 2018

Yep. If this works fine on mobile, then go for it.

@gziolo gziolo merged commit 27a4cea into master Oct 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the fix/link-label branch Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment