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

Backwards Compatibility Improvements? #83

Closed
ryanwiemer opened this issue Apr 8, 2015 · 9 comments
Closed

Backwards Compatibility Improvements? #83

ryanwiemer opened this issue Apr 8, 2015 · 9 comments

Comments

@ryanwiemer
Copy link

Awesome plugin guys. Really great stuff. I was wondering if there are any plans for some type of bulk method to add the responsive capabilities of this plugin to previously added images. I know this can be done one by one but if anyone knows of some type of workaround to apply to all past images that would be great! I have another project with 500+ previously added images that I would like to use this plugin with.

Any thoughts?

@joemcgill
Copy link
Member

Hi @ryanwiemer. This is a tough one. The problem is that WordPress saves the HTML for your images as content in your posts, so we'd have to either rewrite all of the content in your database—which isn't a great idea—or filter every image in your content before if loads for your visitors to add the necessary code. The latter is probably the best approach, but I don't think it would perform well.

At some point we should probably run a few test cases to see exactly how much drag this would put on the content loading.

@jaspermdegroot
Copy link
Member

I have also been looking for a solution for this. Besides adding the attributes to existing images, I also wanted to be able to update and completely remove the attributes. Rewriting the content in the database (because of performance) was my first thought, but because I don't have much experience with that I decided to look into filtering the content first.

My conclusion was that the performance penalty is more or less the same as for post thumbnails. In my test environment that is ~4ms per image with 5 image sizes for the srcset. The code I tested with contained the performance improvement from my PR #82.

Pros:

  1. The same code handles everything I wanted to achieve (existing images, updating, and removal)
  2. No JS for the "image updater" needed anymore, so it fixes JS: Sizes object doesn't contain all image sizes #80 (and Add unit tests for plugin JavaScript #72)
  3. Easy to implement the "use smallest image as src image" solution for Double download in Safari #78
  4. No need to use a data- attribute for sizes anymore

Cons:

  1. Increases execution time
  2. Backwards compatibility; what to do with srcset and data-sizes attributes that are already in the content (my code didn't deal with this at all)
  3. The code isn't suitable anymore to be used as template tag; additional code would be needed to keep this feature

I know that it's a goal of this plugin to become part of WordPress core one day. I am not sure what is the best approach when you keep that in mind. Rewriting content in the database will never land in core but that code can easily be separated from the rest (could even be a separate plugin), while filtering the content is a whole different way of doing things and doesn't really match anymore with how WordPress core handles images in the content (ie. creating all markup when the image is inserted).

Since I need a customize version of this plugin anyway (to make it work with Lazysizes) I decided to continue to use the content filtering solution. The only con that I have to deal with (performance) can be resolved by using a cache plugin. If you want I can create a PR to share my code.

@joemcgill - Although I decided to go for content filtering I am still curious why you think rewriting content in the database isn't a good idea.

@side777
Copy link

side777 commented Apr 13, 2015

I use content filtering too with good results. The reasons why writing the srcset and the sizes to the content / database is not an option for me: image sizes are design dependent and bloat the database with redundant data, the exact same sizes attribute for every single img in every single post. The srcset contains 5 or maybe 10 times the exact same very long absolute path and the exact same image file name and only a little number suffix difference. This is a total waste. Plus: the code gets terribly unreadable.
The whole srcset concept is flawed if you ask me, it clearly belongs in the css, not the markup. Feels like going back to inline styles. (But this is discussed elsewhere.)
So content filtering is my way to go for now.

@joemcgill
Copy link
Member

Although I decided to go for content filtering I am still curious why you think rewriting content in the database isn't a good idea.

@jaspermdegroot I think you almost answered your own question earlier in your post when you mentioned that the goal is to become a core part of WordPress at some point and the core devs are very wary—as they should be—about rewriting user content.

If we can demonstrate that we can set both the srcset and sizes in a filter on page load in a way that is performant, I'd be thrilled to not even have to deal with that rabbit hole. In the mean time, we're doing all the calculation when the post is saved and doing very little filtering on the front end so that users don't experience any lag in page load times on account of the plugin searching the DB for srcset candidates and rewriting the HTML.

On the other hand, as @side777 noted, we definitely don't want to save sizes attributes to the database because that attribute is definitely design (i.e., theme) dependent. I think what we're doing now is working ok, but I'm sure there's a better solution that would be more extendable.

@jaspermdegroot
Copy link
Member

@joemcgill - Ok, thanks. I was just curious if there was something that I didn't think of. I try to stay away from rewriting in the database, hence I don't have experience with it.

I will create a PR to demonstrate content filtering.

@jaspermdegroot
Copy link
Member

I think it's better to wait with creating a PR for this until we decided about the currently open PRs, since my code is based on the changes in those PRs.

@jaspermdegroot
Copy link
Member

Another pro of content filtering is that it makes things work with page builder plugins like Visual Composer. See #89.

Update: See also #160

@bcreeves
Copy link

+1 for this feature. Visual Composer and other drag a drop site builders have proliferated but at the expense of building an extremely bloated site. Being able to add this plugin to one of those sites would be a huge improvement. Maybe if there was a checkbox to turn on content filtering so that it wouldn't be the default?

jaspermdegroot added a commit to jaspermdegroot/wp-tevko-responsive-images that referenced this issue Jul 18, 2015
jaspermdegroot added a commit to jaspermdegroot/wp-tevko-responsive-images that referenced this issue Aug 28, 2015
jaspermdegroot added a commit to jaspermdegroot/wp-tevko-responsive-images that referenced this issue Aug 31, 2015
jaspermdegroot added a commit to jaspermdegroot/wp-tevko-responsive-images that referenced this issue Sep 2, 2015
jaspermdegroot added a commit to jaspermdegroot/wp-tevko-responsive-images that referenced this issue Sep 2, 2015
jaspermdegroot added a commit that referenced this issue Sep 14, 2015
jaspermdegroot added a commit that referenced this issue Sep 15, 2015
jaspermdegroot added a commit that referenced this issue Sep 16, 2015
@joemcgill
Copy link
Member

This has been addressed in #177.

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

No branches or pull requests

5 participants