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

0.9.33 #401

Merged
merged 13 commits into from Jan 17, 2017
Merged

0.9.33 #401

merged 13 commits into from Jan 17, 2017

Conversation

Lofesa
Copy link

@Lofesa Lofesa commented Jan 11, 2017

@khaledMohammed000 @ahmedkaludi @MohammedKaludi If you consider these changes, can be merged

@Lofesa
Copy link
Author

Lofesa commented Jan 15, 2017

Hi @khaledMohammed000 @ahmedkaludi @MohammedKaludi
In this commit I added the do_sorcode() function and supressed the regex to erase Visual Composer shortcodes. With these function all shortcodes inject the html code and the plugins must deal with it. Don´t know how it work whit others shortcodes, they need testing. I have done a test with a page I added VC elements I use regularly and it work.
Examples in a live site:
https://intersindical.intersindicalrm.org/nosotros/amp/
https://intersindical.intersindicalrm.org/intersindical-valenciana-v-congreso/amp/

Copy link
Collaborator

@MohammedKaludi MohammedKaludi left a comment

Choose a reason for hiding this comment

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

We are using better code to render VC code, this code is not required.
Please remove the code.

Copy link
Collaborator

@MohammedKaludi MohammedKaludi left a comment

Choose a reason for hiding this comment

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

Nice idea to give a default value for SD.

I think this code need to be tested to not deal whit shortcodes in a one by one basis.
Copy link
Contributor

@khaledMohammed000 khaledMohammed000 left a comment

Choose a reason for hiding this comment

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

Great 👍

Copy link
Contributor

@khaledMohammed000 khaledMohammed000 left a comment

Choose a reason for hiding this comment

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

Hey @Lofesa

I have gone through https://css-tricks.com/prefetching-preloading-prebrowsing/

We have internally decided to go with "prefetch" instead of both "preconnect" and "dns-prefetch", reason being caching stuff before they are used has more benefits on performance than other options 👍

@khaledMohammed000
Copy link
Contributor

khaledMohammed000 commented Jan 16, 2017

Hey @Lofesa

Please make changes in (just replace "preconnect" with "prefetch" in these 4 files )

  1. e73149f
  2. 07c3ff1
  3. 327751c
  4. 3cdcd5e

and then we are good to Merge 👍

@Lofesa
Copy link
Author

Lofesa commented Jan 16, 2017

Hi @khaledMohammed000

Is not a trivial change. Prefechting the sub-resources must be done at each one not in a general call like the preconnect. Example;

<link rel="prefetch" href="https://cdn.ampproject.org/v0/amp-sidebar-0.1.js" >

and not all those sub-resources are used on all AMP pages.
As far as these sub-resouces belongs on http/2 protocol, when the preconnect is done sub-resources are downloaded as you can test with webpagetest w/o wait to be called in the html and are the 1s thing that are loaded.

When you know what js files must be used need to print the link rel in head and script call in body.
Prefecht and preload are for a file by file basis

@Lofesa
Copy link
Author

Lofesa commented Jan 16, 2017

Changed the Disable comment button when comments are disable #393

@Lofesa
Copy link
Author

Lofesa commented Jan 16, 2017

Hi @khaledMohammed000
More on prefecht vs. preconnect
You don´t need dns-prefecht and preconnect. Only one is needed.
Dns-prefecht do dns search before sub-resources are used, preconnect do this plus ssl negotiation and connect stuff and in http/2 it start to download things from the site we do the preconnect stuff before images or any other sub-resource
And one more thing, prefecht do the download and caching when the browser is idle.

@khaledMohammed000 khaledMohammed000 merged commit 448cfbe into ahmedkaludi:0.9.33 Jan 17, 2017
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.

None yet

3 participants