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

Test patterns against hostname:port instead of just domain #7

Merged
merged 1 commit into from
Dec 12, 2015

Conversation

merkledo
Copy link
Contributor

With this change you can use the indicator for projects on localhost (with just different ports, not hostnames) as well.

Shouldn't have any side effects on the default umbraco.io rules. Otherwise: @merkledo + sorry ;)

With this change you can use the indicator for projects on localhost (with just different ports, not hostnames) as well.
@leekelleher
Copy link
Owner

Thanks @dmerkle! Good idea! I'll review and merge in.

@leekelleher
Copy link
Owner

Hi @dmerkle, sorry, I know this was a couple of months ago, I've only got around to reviewing it.

Do you have an example of the config values?

Since location.host contains the port number, I think we'll need to amend the default regex for "localhost"?

@merkledo
Copy link
Contributor Author

Hi @leekelleher, do you mean like these:
var config = [
{ pattern: "^localhost:8010$", color: "1d1d99" },
{ pattern: "^localhost:8020$", color: "1d991d" },
{ pattern: "^localhost:8030$", color: "991d1d" }
];

@leekelleher
Copy link
Owner

Thanks @dmerkle. I'm wondering what the default value for "localhost" should be. It is currently "^localhost$", but with port numbers being used as part of the RegEx match, it wouldn't work out of the box.

I'll experiment with it when I next get chance to work on the code 👍

leekelleher added a commit that referenced this pull request Dec 12, 2015
Test patterns against hostname:port instead of just domain
@leekelleher leekelleher merged commit 09ba0e6 into leekelleher:master Dec 12, 2015
@leekelleher
Copy link
Owner

I finally got around to merging this in, thanks @merkledo! Apologies for my long delay.

@merkledo
Copy link
Contributor Author

Nice! @leekelleher thanks for merging.

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

Successfully merging this pull request may close these issues.

None yet

2 participants