Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Handle different protocol prefixes in auto-suggest #5092

Merged
merged 1 commit into from
Oct 24, 2016

Conversation

aekeus
Copy link
Member

@aekeus aekeus commented Oct 24, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
    • Remove http and https from URLs when sorting
    • Increase history limit to 3, suggested to 3
    • Update unit tests

Auditors: @bbondy

Test Plan:

  1. Visit a mixture of sites that have a similar name (twitter.com, twilio.com) but have a different protocol prefix (http, https)
  2. Ensure that history sorting is entirely based on count and frequency
  3. Ensure that a maximum of 3 history items and 3 suggested sites are shown (so as not to overflow the space available and cause a scrollbar to appear)

Fixes: #5091

  * Remove http and https from URLs when sorting
  * Increase history limit to 3, suggested to 3
  * Update unit tests

Auditors: @bbondy

Test Plan:

  1. Visit a mixture of sites that have a similar name (twitter.com, twilio.com) but have a different protocol prefix (http, https)
  2. Ensure that history sorting is entirely based on count and frequency
  3. Ensure that a maximum of 3 history items and 3 suggested sites are shown (so as not to overflow the space available and cause a scrollbar to appear)

Fixes: #5091
@aekeus aekeus added this to the 0.12.7dev milestone Oct 24, 2016
@@ -107,5 +107,8 @@ module.exports.simpleDomainNameValue = (site) => {
*
*/
module.exports.normalizeLocation = (location) => {
return location.replace(/www\./, '')
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to use the window.URL object?
https://developer.mozilla.org/en-US/docs/Web/API/Window/URL

Also, another consideration would be if it makes sense to put the logic into our URL utils:
https://github.com/brave/browser-laptop/blob/master/js/lib/urlutil.js

Copy link
Member

Choose a reason for hiding this comment

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

++ for extra logic in url utils but I'll merge this as is for now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix auto-suggest sort order with different protocol prefixes
6 participants