search

Scripler / scripler

The Scripler web application source code repository
Do What The F*ck You Want To Public License
5 stars 1 forks source link

Issue 859 new fixes #1028

Closed roxanastolniceanu closed 9 years ago

roxanastolniceanu commented 9 years ago

@allanmc I fixed some things regarding hyperlinks: see the commits and the Trello card. I still couldn't manage to hide the error, maybe you can take a look.

allanmc commented 9 years ago

@roxa0063 So this is not a review, but just a pull-request i can continue working on? Or what do you mean:)?

roxanastolniceanu commented 9 years ago

@allanmc You can review the other commits I made if you don't want to continue with the remaining issue. I'll merge these and then leave the last fix for later... it's up to you :)

allanmc commented 9 years ago

@roxa0063 Okay your changes looks good - does David accept the current state? If not then we should not merge until i fix the remaining issue.

Just one minor note - your https regex. Instead of (https*?) it should be (https?), unless i'm missing something.

roxanastolniceanu commented 9 years ago

@allanmc I want the s to be optional, so "http" is a valid one as well. Doesn't the '*' mean 0 or more?

allanmc commented 9 years ago

"*" means 0 or more "?" means 0 or 1 Thats why https? should be fine :) Your regex allows httpsssssssssss :)

2015-06-03 13:18 GMT+02:00 roxa0063 notifications@github.com:

@allanmc https://github.com/allanmc I want the s to be optional, so "http" is a valid one as well. Doesn't the '*' mean 0 or more?

— Reply to this email directly or view it on GitHub https://github.com/Scripler/scripler/pull/1028#issuecomment-108306008.

roxanastolniceanu commented 9 years ago

@allanmc Ok, point taken. I changed it and David said it's ok to commit this now and I will move the card to "Needs Estimation..." afterwards for the remaining issue.