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

Compatibility/smart slider #145

Closed
wants to merge 12 commits into from
Closed

Conversation

preda-bogdan
Copy link
Contributor

No description provided.

Copy link
Contributor

@selul selul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inc/lazyload_replacer.php Show resolved Hide resolved
inc/manager.php Outdated
@@ -481,7 +481,7 @@ public function process_urls_from_content( $html ) {
* @return array
*/
public function extract_image_urls_from_content( $content ) {
$regex = '/(?:http(?:s?):)(?:[\/\\\\|.|\w|\s|-])*\.(?:' . implode( '|', array_keys( Optml_Config::$extensions ) ) . ')(?:\?{1}[\w|=|&|\-|\.|:|;]*)?/';
$regex = '/((?:http(?:s?):)(?:[\/\\\\|.|\w|\s|-])*\.(?:' . implode( '|', array_keys( Optml_Config::$extensions ) ) . ')(?:\?{1}[\w|=|&|\-|\.|:|;]*)?)|((?:[\/\\\\|.|\w|\s|-])*\.(?:' . implode( '|', array_keys( Optml_Config::$extensions ) ) . ')(?:\?{1}[\w|=|&|\-|\.|:|;]*)?)/';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems not right if we try to do target in regex both with and without schema urls, why we don't use just the regex without the schema to search.

Also, how the re-written urls will look, they will include the schema of the original url?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i will test that

yes, they will include the schema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GrigoreMihai You can check the regex here https://regex101.com/ and add the link with the tests to the conversation.

Copy link
Contributor

@GrigoreMihai GrigoreMihai Oct 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preda-bogdan added a commit that referenced this pull request Sep 23, 2019
selul added a commit that referenced this pull request Sep 23, 2019
selul pushed a commit that referenced this pull request Sep 25, 2019
selul added a commit that referenced this pull request Sep 25, 2019
selul pushed a commit that referenced this pull request Sep 25, 2019
#### [Version 2.1.2](v2.1.1...v2.1.2) (2019-09-25)

* **Bug Fixes**
   * adds preconnect hint for image domain and js library domain ([11b697d](11b697d))
   * compatibility with cache_enabler [#136](#136) ([483262f](483262f))
   * improve checking for editing context when the replacement should be off ([e7510f6](e7510f6))
   * lazyload query urls part of [#145](#145) ([a048f68](a048f68))
   * preload lazyload js file when lazyload setting is active ([828e1de](828e1de))
   * remove replacement on Divi theme builder ([86ab6d2](86ab6d2))
   * replacement was not working for urls with special chars ([48a4966](48a4966))
   * replacing url's with query strings without the query in the modified url [#141](#141) ([0025559](0025559))
   * replacing url's with regex in <a> tags [#141](#141) ([4b2264f](4b2264f))
   * resource hints condition check [skip release] ([a0f30e7](a0f30e7))

* **Features**
   * adds retina settings control which enable/disable serving of HiDPI images ([73c8712](73c8712))
   * adds visitors based plan integration ([ea07a94](ea07a94))
@nextend
Copy link

nextend commented Oct 30, 2019

I'm the developer of Smart Slider 3, if I can help you with this feature let me know.

@selul
Copy link
Contributor

selul commented Oct 30, 2019

@nextend from our findings Smart Slider 3 uses schemaless URLs for images, making impossible for optimole to target them and replace the URLs.
Which is the best way to fix this with Smart Slider?

If there any filter which we can use to alter the final src of an image from slider with our URL format before you output it in the HTML?

@nextend
Copy link

nextend commented Oct 30, 2019

Schemaless is a setting in Smart Slider. It is enabled by default, but can be disabled.

I do not know too much about your plugin. Basically your goal is to replace 's src attribute with your own url. Can you work with background images as well?

Under the hood the following happens in Smart Slider when you display a slider on the frontend:

  1. Checks if the slider's HTML cached and the cache is valid
  2. If the cache invalid -> the HTML code of the slider generated and displayed
  3. If the cache valid -> just display the HTML code

In step 2, Smart Slider could apply_filters for the image url's, but that would mean that the result is cached. So if something changes in your plugin or your plugin deactivated, the cache would be still valid with wrong urls.

In step 3, Smart Slider could apply_filters, but you would need to find the image urls with regexp, which I think you want to avoid as it is not simple task. This applies when you regexp the whole document without the filter. If you need Smart Slider could give a list of the images used in that specific slider (We have this info cached for Jetpack).

I saw in the commits that you are looking for data-desktop="" data-tablet ="" and such attributes. Those are used for a display:none image which will be used as background images. So the image tag itself never loads those images. It is there for user who wants their slide backgrounds to show for search engines. Based on your comment in #148, we might need to add that specific exclude class only these hidden images to skip them and you would be able to find other images and replace them with your SVGs.

@selul
Copy link
Contributor

selul commented Oct 30, 2019

@nextend can we turn the schema-less setting off from Optimole, like hooking to a filter that changes the returned value of the setting, when people chose the serve images from our side?

Thanks !

@selul selul deleted the compatibility/smart_slider branch December 9, 2019 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants