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

Feature: add https to link filtering #22

Merged
merged 3 commits into from
Feb 11, 2015
Merged

Conversation

jamesjwarren
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 67.32% when pulling 537d45b on feature/link-filter into be47b91 on develop.

@@ -9,7 +9,7 @@ angular.module("thisissoon.core").filter("linkDisplay", [
"$filter",
function($filter) {
return function(input) {
return input.replace("http://", "").replace("www.", "");
return input.replace("http://", "").replace("https://", "").replace("www.", "");
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jamesjwarren would it not be better to use regex for this? something like: /[a-zA-Z]+:\/\// would match any protocol and: /w{3}./would match www.

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.15%) to 66.17% when pulling de3a8a7 on feature/link-filter into be47b91 on develop.

@@ -9,7 +9,7 @@ angular.module("thisissoon.core").filter("linkDisplay", [
"$filter",
function($filter) {
return function(input) {
return input.replace("http://", "").replace("www.", "");
return input.replace(/.*?:\/\//g, "").replace(/w{3}./, "").replace(/\/.*/g, "");
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jamesjwarren have you tested this? this is a classic case for unit testing

Copy link
Member Author

Choose a reason for hiding this comment

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

There are tests for this here: https://github.com/thisissoon/thisissoon-frontend/blob/feature/link-filter/tests/unit/thisissoon-core/filters/linkDisplay.js but I could probably add a few more cases. I'll do that now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool, good job 👍

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.15%) to 66.17% when pulling c9cb5e3 on feature/link-filter into be47b91 on develop.

jamesjwarren added a commit that referenced this pull request Feb 11, 2015
Feature: add https to link filtering
@jamesjwarren jamesjwarren merged commit a109ec1 into develop Feb 11, 2015
@jamesjwarren jamesjwarren deleted the feature/link-filter branch February 12, 2015 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants