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

[FIX] web_editor: lazy load the wysiwyg #30700

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@Gorash
Copy link
Contributor

Gorash commented Jan 30, 2019

Issue: wysiwyg asset slow down the loading of the website, error
inadvertently introduced: #29775

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@Gorash Gorash requested a review from VincentSchippefilt Jan 30, 2019

@robodoo robodoo added the CI 🤖 label Jan 30, 2019

@C3POdoo C3POdoo added the RD label Jan 30, 2019

@ged-odoo

This comment has been minimized.

Copy link
Contributor

ged-odoo commented Jan 31, 2019

robodoo delegate=@VincentSchippefilt

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch Jan 31, 2019

@robodoo robodoo removed the CI 🤖 label Jan 31, 2019

@VincentSchippefilt VincentSchippefilt force-pushed the odoo-dev:master-wysiwyg-load-chm branch 2 times, most recently to f0fb9c9 Jan 31, 2019

@robodoo robodoo added the CI 🤖 label Jan 31, 2019

@qsm-odoo
Copy link
Contributor

qsm-odoo left a comment

@Gorash Suggestion: rename the actual assets bundle assets_wysiwyg instead of wysiwyg_assets to follow the same convention than other assets. Also I would rename the template containing the t-call-assets compiled_assets_wysiwyg instead of wysiwyg.

If this can be merged for the freeze and if I can take a quick look at the final stuff before, it would be even greater. Thanks! :)

@qsm-odoo
Copy link
Contributor

qsm-odoo left a comment

Some more suggestions :)

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch from f0fb9c9 Feb 4, 2019

@robodoo robodoo removed the CI 🤖 label Feb 4, 2019

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch Feb 4, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 4, 2019

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch Feb 4, 2019

@robodoo robodoo added the CI 🤖 label Feb 4, 2019

@VincentSchippefilt VincentSchippefilt force-pushed the odoo-dev:master-wysiwyg-load-chm branch Feb 5, 2019

@robodoo robodoo removed the CI 🤖 label Feb 5, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Feb 6, 2019

Final review and merge tomorrow? :)

@qsm-odoo
Copy link
Contributor

qsm-odoo left a comment

Could somebody adapt and test this please? @Zinston ?
I'd really like this to be in saas-12.2, whether it should be bug-fixed or not. Website pages take about 2x longer to be shown without this.

Show resolved Hide resolved addons/mass_mailing/views/editor_field_html.xml Outdated
Show resolved Hide resolved addons/web_editor/views/editor.xml Outdated
Show resolved Hide resolved addons/website/static/src/scss/website.editor.ui.scss Outdated
Show resolved Hide resolved addons/website/static/src/scss/website.editor.ui.scss Outdated
Show resolved Hide resolved addons/website/static/src/scss/website.editor.ui.scss Outdated
Show resolved Hide resolved addons/website/views/website_templates.xml Outdated
@Zinston

This comment has been minimized.

Copy link

Zinston commented Feb 8, 2019

Could somebody adapt and test this please? @Zinston ?
I'd really like this to be in saas-12.2, whether it should be bug-fixed or not. Website pages take about 2x longer to be shown without this.

I haven't been following this task but I'll take a look at it. When you say adapt, do you simply mean to review and apply your requested changes or is there more?

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Feb 8, 2019

@Zinston Thanks. Don't know if there is more, I have not tested the branch ;) I think the freeze is for monday though, I suppose @Gorash will be able to take a final look maybe...

@Zinston Zinston force-pushed the odoo-dev:master-wysiwyg-load-chm branch to ffaa110 Feb 8, 2019

@robodoo robodoo added the CI 🤖 label Feb 8, 2019

@Zinston

This comment has been minimized.

Copy link

Zinston commented Feb 8, 2019

@qsm-odoo Many things are broken in CSS (eg "Customize" button), this requires a big fix (likely due to the bootstrap overrides)... Other than that, I fixed the rebase errors that broke the branch and things seem to work OK. Hopefully @Gorash can fix the CSS on Monday and it can be merged. I'll test again on Monday as well.

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Feb 8, 2019

Mmh, I think it is because the css is lazy loaded only when the editor is instantiated... but many website features requires this CSS, once connected.

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch from ffaa110 to ef9ca17 Feb 11, 2019

@robodoo robodoo removed the CI 🤖 label Feb 11, 2019

@robodoo robodoo removed the CI 🤖 label Feb 13, 2019

@VincentSchippefilt

This comment has been minimized.

Copy link
Contributor

VincentSchippefilt commented Feb 13, 2019

robodoo r+

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Feb 13, 2019

I am scared, the website ace editor / toggles / ... are broken on runbot. Also trying to go on website produces a traceback... fix incoming ?

Mmh, I think it is because the css is lazy loaded only when the editor is instantiated... but many website features requires this CSS, once connected.

@VincentSchippefilt

This comment has been minimized.

Copy link
Contributor

VincentSchippefilt commented Feb 13, 2019

robodoo r-

@robodoo robodoo removed the r+ 👌 label Feb 13, 2019

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch from 44a6348 to 07f0447 Feb 13, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 13, 2019

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch from 07f0447 to e4fa992 Feb 14, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 14, 2019

@VincentSchippefilt

This comment has been minimized.

Copy link
Contributor

VincentSchippefilt commented Feb 14, 2019

robodoo r+

@robodoo robodoo added the r+ 👌 label Feb 14, 2019

robodoo pushed a commit that referenced this pull request Feb 14, 2019

[IMP] web_editor: lazy load the wysiwyg
Issue: wysiwyg asset slow down the loading of the website, error
inadvertently introduced: #29775

The assets are now loaded assynchroneously, when the editor is needed,
its assets will be loaded.

closes #30700
@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 14, 2019

Linked pull request(s) odoo/enterprise#3552 not ready. Linked PRs are not staged until all of them are ready.

[IMP] web_editor: lazy load the wysiwyg
Issue: wysiwyg asset slow down the loading of the website, error
inadvertently introduced: #29775

The assets are now loaded assynchroneously, when the editor is needed,
its assets will be loaded.

@Gorash Gorash force-pushed the odoo-dev:master-wysiwyg-load-chm branch from e4fa992 to 118ea74 Feb 14, 2019

@VincentSchippefilt

This comment has been minimized.

Copy link
Contributor

VincentSchippefilt commented Feb 14, 2019

robodoo r+

robodoo pushed a commit that referenced this pull request Feb 14, 2019

[IMP] web_editor: lazy load the wysiwyg
Issue: wysiwyg asset slow down the loading of the website, error
inadvertently introduced: #29775

The assets are now loaded assynchroneously, when the editor is needed,
its assets will be loaded.

closes #30700

@robodoo robodoo added merged 🎉 and removed merging 👷 labels Feb 14, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

robodoo commented Feb 14, 2019

Merged, thanks!

@robodoo robodoo closed this Feb 14, 2019

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