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

Don't show protocol on suggestions in navigation #20350

Merged
merged 2 commits into from
May 18, 2020

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Feb 21, 2020

Closes #19670

Description

On suggestions of navigation, protocols won't be shown.

How has this been tested?

  • I added test case in LinkControl test.
  • Test Env: Ubuntu 18.04 Node 12.14.1
  • It's a simple change in LinkControl. It doesn't affect others.

Screenshots

Before:

Screenshot from 2020-02-21 12-03-23

After:

Screenshot from 2020-02-21 12-09-30

Types of changes

Simple fix. (I don't think it's either a bug or a new feature) It doesn't break anything.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • [N/A] I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@sainthkh sainthkh requested a review from getdave February 21, 2020 03:17
@sainthkh sainthkh changed the title Don't show protocol on suggestions of navigation Don't show protocol on suggestions in navigation Feb 21, 2020
Comment on lines +49 to +53
{ ! isURL &&
( filterURLForDisplay(
safeDecodeURI( suggestion.url )
) ||
'' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

filterURLForDisplay() also strips off www in addition to protocols. I'm not sure that's what we want here.

Copy link
Member

@aduth aduth Apr 2, 2020

Choose a reason for hiding this comment

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

It might not be what was prescribed by #19670, but if how we're presenting the link here aligns to the purpose of how a function like filterURLForDisplay is expected to be used, then we should embrace the consistency that comes with it. We might decide that trimming www. is not something we want to do for URLs filtered for display, but it's a change which should happen in the implementation of filterURLForDisplay, and not a decision we should want to make at any particular instance of where that function is used.

Or, if it is a decision that depends on the context of where the URL is presented, then we can incorporate that as an option of the function. I'm inclined to think we should try to avoid that if possible, for the sake of consistency (URLs appearing same everywhere), simplicity (smaller function signature), and maintainability (needing to make decisions is a hurdle we can often avoid).

@earnjam
Copy link
Contributor

earnjam commented Mar 20, 2020

There didn't seem to be a consensus in #19670 about whether this was the right thing to do. For the recent posts from the current site, we may not need to show a domain at all since it's implied and should be known. But for direct entry URLs, we probably should keep the full thing, including protocols.

@aduth
Copy link
Member

aduth commented Apr 2, 2020

There didn't seem to be a consensus in #19670 about whether this was the right thing to do.

I'm personally a bit conflicted:

  • On one hand, I feel it makes it appear less obvious as a URL. To me, if we want to have a URL in this preview, then it should appear as a URL.
  • On the other hand, we have this function filterURLForDisplay which presumably exists to control how we want URLs to look in a UI context. On that basis alone, processing it through the function makes sense. And it could even allow us to change how we display URLs by simply adjusting the implementation of filterURLForDisplay.

It probably should have been added to #19670, but I'll add the "Needs Design Feedback" since I think it would help to solicit additional design feedback / perspective here.

@aduth aduth added [Block] Navigation Affects the Navigation Block [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) Needs Design Feedback Needs general design feedback. labels Apr 2, 2020
@karmatosed
Copy link
Member

I think visually I prefer it as it is obvious it's URL. Let's get this in and code reviewed. I think because of where it is and format, it's obviously a URL.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label May 18, 2020
@aduth aduth force-pushed the fix/19670-no-protocol branch from 6af81e0 to 7f1bf39 Compare May 18, 2020 16:39
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

The branch is quite old, so I've rebased it to ensure everything's still in order against the latest master. Otherwise, the code looks solid 👍

@aduth aduth merged commit 31bb9a6 into WordPress:master May 18, 2020
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LinkControl - remove URL protocol from Entity (only!) search results
4 participants