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.

Sign up for GitHub

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

Jump to bottom

website_lazy_load_image: fix doctype missing (closes #718) #719

Closed
wants to merge 4 commits into from
Closed

website_lazy_load_image: fix doctype missing (closes #718) #719

wants to merge 4 commits into from

Conversation

flotho
Copy link
Member

@flotho flotho commented May 12, 2020 •
edited
Loading

Fix #718 and probably odoo/odoo#44612

@simahawk simahawk added this to the 12.0 milestone May 14, 2020
@flotho flotho changed the title FIX #718 , doctype not displayed FIX #718 , doctype not displayed with lazy load image Jun 25, 2020
tarteo
tarteo approved these changes Jul 2, 2020
View reviewed changes
Copy link
Member

@tarteo tarteo left a comment

Choose a reason for hiding this comment

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

The doctype is persisting with this fix.

Before:
image

After:
image

@tarteo tarteo mentioned this pull request Jul 2, 2020
Closed
@simahawk
Copy link

simahawk commented Jul 3, 2020

@flotho can you check the build?

HaraldPanten
HaraldPanten approved these changes Jul 3, 2020
View reviewed changes
Copy link

@HaraldPanten HaraldPanten left a comment

Choose a reason for hiding this comment

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

Small fix, seems to be OK

@flotho flotho force-pushed the patch-1 branch 4 times, most recently from 7c03bd4 to 0bbcfa4 Compare July 5, 2020 14:21
@flotho
FIX OCA#718 , doctype not displayed
e5d1016 
and probably odoo/odoo#44612

Update ir_ui_view.py

fix lint

add tests with doctype

fix useless spaces

more pylint fix

More pylint fix

Maybe the last one on pylint ;-)
@flotho
Copy link
Member Author

flotho commented Jul 5, 2020

Hi @simahawk , @HaraldPanten ,

More tests have been added and travis is green, the codecov is not concerning my PR.
Is a merge possible ?

(cc @pedrobaeza , I finally succeed in squashing and respecting pylint !!! so happy )

flotho and others added 2 commits July 5, 2020 21:35
@flotho
FIX OCA#718 , doctype not displayed
c8bac08 
and probably odoo/odoo#44612

Update ir_ui_view.py

fix lint

add tests with doctype

fix useless spaces

more pylint fix

More pylint fix

Maybe the last one on pylint ;-)
@flotho
Merge branch 'patch-1' of https://github.com/flotho/website into patch-1
40d008f
simahawk
simahawk requested changes Jul 6, 2020
View reviewed changes
Copy link

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

thnx for adding tests!

website_lazy_load_image/tests/test_lazy_load_image.py Outdated Show resolved Hide resolved
website_lazy_load_image/__manifest__.py Outdated Show resolved Hide resolved
@flotho
FIX: Correct unit test
92c8271 
FIX: version unbump
@flotho
Copy link
Member Author

flotho commented Aug 7, 2020

Hi @simahawk ,

I need a little help on this one. I'm struggling on the test. It works locally with a non demo database and it failed on runbot but I'm not able to understand why and how to handle this case.

Regards

@simahawk simahawk mentioned this pull request Dec 3, 2020
Closed
@simahawk simahawk changed the title FIX #718 , doctype not displayed with lazy load image website_lazy_load_image: fix doctype missing (closes #718) Dec 3, 2020
@ypapouin
Copy link

ypapouin commented Dec 3, 2020

@simahawk, thanks for the routing to the right issue/fix.
Ok for the fix but I'm not sure that putting test on this module is relevant as any module hijacking the final rendering can break everything. Don't you think that it should be moved as a base/web test in OCB instead ?

@simahawk
Copy link

simahawk commented Dec 3, 2020 •
edited
Loading

@ypapouin we must make sure THIS module which hacks the DOM works properly.
If any other module is messing up w/ the DOM it should do the same.
What we could do is: have a base module like qweb_render_hook or base_render_hook or alike which provides the base features and tests utils to handle such case.
But for now I'd fix the tests here and add maybe a TODO in the roadmap to split it later.

@ypapouin
Copy link

Ok some tests could be needed but fixing this module should be a priority. Since this PR exists for more than 6 months it would be fair to split this PR (KISS mode) : one for the fix (that could be approved and merged quickly) and the other one for the test that way the PR for the test could stay here until it is fixed.

@simahawk simahawk mentioned this pull request Dec 15, 2020
Merged
@simahawk
Copy link

let's see if tests pass here #810

@simahawk
Copy link

replaced by #810

@simahawk simahawk closed this Dec 15, 2020
@flotho
Copy link
Member Author

flotho commented Dec 16, 2020

@simahawk , thanks for helping, your PR helps me to understand mochrequest.

Regards

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

@simahawk simahawk simahawk requested changes

@tarteo tarteo Awaiting requested review from tarteo

@HaraldPanten HaraldPanten Awaiting requested review from HaraldPanten

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
12.0
Development

Successfully merging this pull request may close these issues.

v12 - Doctype tml is missing
6 participants
@flotho @simahawk @ypapouin @tarteo @HaraldPanten @OCA-git-bot