search

kthornbloom / Smoothproducts

A simple, lightweight and responsive product image viewer using jQuery
kthornbloom.com/smoothproducts
185 stars 89 forks source link

double quoting url #8

Closed m-hume closed 9 years ago

m-hume commented 9 years ago

With version 2.0.1 i was running into a problem where background image urls were being extracted from url("path/image.jpg") the url( and ) were removed leaving a double quoted string in the variable "path/image.jpg" This was then being pasted to an image src producing <img src=""path/image.jpg""/> Im seeing this in Windows based Firefox 36.0.1 and Internet Explorer 11.0 Google Chrome 41.0.2272.89 m does not exhibit this issue

kthornbloom commented 9 years ago

Thanks, that looks great.

m-hume commented 9 years ago

Hi Kevin Can i firstly say your smoothproducts plugin is great! Looks and functions well. Just what i was looking for I do have a few issues/ideas that i'd like to implement thou I'll just go ahead and list them

  1. With javascript disabled .sp-wrap is hidden by the css. No need for this imho
  2. Your loading image is hidden as soon as the dom is loaded. This does not mean the images have loaded its just the page structure that has. Was this intended?
  3. Images displayed in sp-lightbox should be navigable using the cursor keys
  4. On an touch enabled device sp-lightbox should be navigable by swipes.
  5. On an touch enabled device there's currently no way to exit the lightbox other than refreshing the page.

Hope this finds you well and you don't mind the pull requests You can always ignore them ;D jon

On 23 March 2015 at 14:52, Kevin Thornbloom notifications@github.com wrote:

Thanks, that looks great.

— Reply to this email directly or view it on GitHub https://github.com/kthornbloom/Smoothproducts/pull/8#issuecomment-85034647 .

kthornbloom commented 9 years ago

Hi Jon, I appreciate when people point out mistakes or suggest improvements, so keep the pull requests coming!

With javascript disabled .sp-wrap is hidden by the css. No need for this imho Since this plugin is (supposed to) only run once images have fully loaded, I didn't want to have an in-between state where you might see a bunch of large images before the script is able to put them where they are supposed to go.

Your loading image is hidden as soon as the dom is loaded. This does not mean the images have loaded its just the page structure that has. Was this intended? Hmm, doesn't calling $(window).load() wait until images have downloaded?

Images displayed in sp-lightbox should be navigable using the cursor keys Sure, that would be easy enough to add

On an touch enabled device sp-lightbox should be navigable by swipes. That would be nice! I just didn't want to include another dependency for that, and probably don't have time to code it from scratch right now.

On an touch enabled device there's currently no way to exit the lightbox other than refreshing the page. You should be able to just tap anywhere except the left or right button, but I suppose that's not obvious!

m-hume commented 9 years ago

Hey Kevin Hi Jon, I appreciate when people point out mistakes or suggest improvements, so keep the pull requests coming!

With javascript disabled .sp-wrap is hidden by the css. No need for this imho

_Since this plugin is (supposed to) only run once images have fully loaded, I didn't want to have an in-between state where you might see a bunch of large images before the script is able to put them where they are supposed to go._Ok for safeties sake this line should be after your include css in the head (noscript tags are valid in the head for html5)

Your loading image is hidden as soon as the dom is loaded. This does not mean the images have loaded its just the page structure that has. Was this intended? Hmm, doesn't calling $(window).load() wait until images have downloaded? My Apologies! The way i was implementing your script was at fault not your code :\

Images displayed in sp-lightbox should be navigable using the cursor keys Sure, that would be easy enough to add

On an touch enabled device sp-lightbox should be navigable by swipes. That would be nice! I just didn't want to include another dependency for that, and probably don't have time to code it from scratch right now. https://github.com/thebird/Swipe is licensed under mit and shouldn't be to hard to modify and shoehorn in. As soon as i get some free time i'll get on it

On an touch enabled device there's currently no way to exit the lightbox other than refreshing the page. You should be able to just tap anywhere except the left or right button, but I suppose that's not obvious! With further testing what you say is true on Android but not in iOS. I'll track it down. Also in index.html you have three meta tags before you open the head tag. These should be inside the head tag Thanks for sharing jon

kthornbloom commented 9 years ago

I welcome any help with making this plugin even more useful! Quick question- what iOS device are you testing with that doesn't close the lightbox? I'm trying on an iPad and it seems to work well.

m-hume commented 9 years ago

IPad mini but using icab web browser Thought they all used the same web kit but apparently not If I find a cure I'll let you know j

On 24 Mar 2015, at 14:45, Kevin Thornbloom notifications@github.com wrote:

I welcome any help with making this plugin even more useful! Quick question- what iOS device are you testing with that doesn't close the lightbox? I'm trying on an iPad and it seems to work well.

— Reply to this email directly or view it on GitHub.