search

drewbrolik / Responsive-Img

Responsive Img is a jQuery plugin that changes an image's src attribute based on its container's width.
207 stars 36 forks source link

Problem with SEO Urls and baseURL #5

Open ChrisKam opened 10 years ago

ChrisKam commented 10 years ago

Hi there,

thank you very much for programming this great plugin; I'm planning to use it on my personal site. I did run into issues with the pathToPHP option: Since my CMS is using SEO urls like index.php/category/subcategory/article, I have to set up the path with a "/" at the beginning - thankfully jQuery is quite smart about this and it works.

However, due to the way the script tries to determine the baseURL by counting the slashes in the path, this obviously breaks the plugin. For me, this was easy to patch and the script is running just fine now, but maybe this is something you want to make a bit more solid if you plan another release. The easiest solution here would to not count slashes at the beginning (and I could easily put in a pull request for this), but I think the whole integration of the PHP file by using baseURL is a bit sketchy - the php for example could never be in a directory above the output etc.

drewbrolik commented 10 years ago

Hey,

You're totally right. Originally, I planned to revisit that part of the plugin. I knew there was potential for problems and that it wasn't the best solution.

As time went on, though, I forgot all about it. Surprisingly, you're the first time I've heard of it being an issue. I imagine others have had problems, though, and they just didn't tell me.

If you wanted to put in a pull request, that would be totally welcome and appreciated, but I agree that the whole baseURL thing is a little sketchy... I should really rethink that whole part.

Thanks for the compliment, though, and I'm glad you got it to work regardless of the issue.

Drew

ChrisKam commented 10 years ago

Hi Drew,

thank you for your thorough response - I put some thoughts into this and came up with a solution: Dealing with path issues of this sort seems to be a real pain on the client, so I shifted that responsibility to the server-side, putting the JS and PHP into one PHP file that serves either JS or PHP based on the "makeImage" request being set or not. That way, the "pathToPHP " variable and its associated code can be dropped entirely, which I think is quite nice.

The only additional code necessary is some function getting the relative path from script to image that is a bit smarter than just counting the "/"s. I am using this one http://stackoverflow.com/a/2638272 and it seems to work fine. Shifting the path determination to the server makes the script now also work with images above the base directory and also with URLs including http://domain.tld (a case that also breaks the current version).

If you're OK with this slightly drastic approach in general, I'd be happy to submit a pull request. I'm just fairly new to the whole Github thing and I wanted to ask first. My code is probably not terrific as I'm doing mostly design and a bit of front-end JS but hopefully, this helps.

best wishes Chris

drewbrolik commented 10 years ago

Hey, that sounds pretty promising. I would certainly be open to you submitting a pull request. I like what you're thinking, and don't worry if your code isn't terrific, as I'll edit anything that needs editing before merging it in.

One thing I'll say upfront, though, is that it might take me a while to look at it and edit/merge it. I'm very busy with work stuff right now. Hopefully that doesn't deter you though!

Thanks! Let me know if you have any questions.

ChrisKam commented 10 years ago

Hi Drew,

thank you for your answer - I've submitted my first pull request, hope this makes sense. I dropped the idea of merging the JS and PHP into one file again after this created hick-ups in my own deploy process with minification and file concatenation.

There is one additional thing I have noticed: You might want to make the user of this script aware that they should protect the PHP file with an .htpasswd for security reasons since the PHP portion is simply called by the client, everyone could just tell it to create a ton of images on the server (or overwrite existing ones with a very low quality)... I think that would be the most comfortable solution; when I put in new images as the author, I can just refresh the page at all the different sizes once and enter the password.

Have a nice week!

best wishes Chris

drewbrolik commented 10 years ago

Hey, I just wanted you to know I haven't had a chance to look at this yet.

I didn't forget you, though!

Drew

ChrisKam commented 10 years ago

Hi Drew, no worries and thank you for letting me know!

best Chris