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

[FIX] portal: odd sidebar on IE11 #32484

Closed

Conversation

Projects
None yet
7 participants
@Julien00859
Copy link
Contributor

commented Apr 8, 2019

Open a quotation from the portal using IE11. The sidebar is rendered as
it did not have any width.

What we are trying to achieve on large screens is to create a sidebar
with a fix width the size of its children and to let the rest of the
content (right to the sidebar) expands. On small screens we want to
invert the direction from left-to-right to up-to-bottom.

The classes col-12, col-lg and flex-lg-grow-0 are used to fulfill
that purpose.

The problem is due to the rule flex-grow: 0 !important applied by the
selector .flex-lg-grow-0 on the sidebar. On Chrome/Firefox the dom
element takes the width of his children as minimal width, the
flex-grow: 0 does not shrink the element bellow that minimal width.
IE11 does not set a minimal width based on the child elements of the
node, the width is equal to 0 and the flex-grow: 0 forbid it from
growing.

The solution is to replace the said classes by the col-lg-auto class
that does exactly what we want and is cross-browser.

opw-1944188
opw-1935087

@Julien00859 Julien00859 requested review from stefanorigano and qsm-odoo Apr 8, 2019

@robodoo robodoo added the seen 🙂 label Apr 8, 2019

@Julien00859 Julien00859 force-pushed the odoo-dev:12.0-opw-1944188-ie11_quotation-juc branch 2 times, most recently from 576f52f to e153eb5 Apr 8, 2019

@C3POdoo C3POdoo added the OE label Apr 8, 2019

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

@@ -182,6 +182,12 @@ hr {
background-color: inherit;
}

// IE 11 hack
// http://browserhacks.com/#hack-d19e53a0fdfba5ec0f283ae86175a3af
*::-ms-backdrop, :root, .flex-lg-grow-0 {

This comment has been minimized.

Copy link
@stefanorigano

stefanorigano Apr 8, 2019

Contributor

@Julien00859 , I think that this selector it's too much generic.
It will affects all the .flex-lg-grow-0 elements, even outside portal (the file is included in the frontend bundle). I suggest to restrict to the sidebar only.

@Julien00859 Julien00859 force-pushed the odoo-dev:12.0-opw-1944188-ie11_quotation-juc branch from e153eb5 to 88f0717 Apr 8, 2019

@Julien00859 Julien00859 requested a review from stefanorigano Apr 8, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 8, 2019

@@ -182,6 +182,12 @@ hr {
background-color: inherit;
}

// IE 11 hack
// http://browserhacks.com/#hack-d19e53a0fdfba5ec0f283ae86175a3af
*::-ms-backdrop, :root, .o_portal_sale_sidebar .flex-lg-grow-0 {

This comment has been minimized.

Copy link
@stefanorigano

stefanorigano Apr 8, 2019

Contributor

Better, but since the fix it's really specific (252px), I'd prefer to affect the element only (adding a class maybe?).

Did you already checked if it's ok on Edge?

This comment has been minimized.

Copy link
@Julien00859

Julien00859 Apr 8, 2019

Author Contributor

Work fine on Edge.

Is a new class really neede ? The only elements in odoo using the .o_portal_sale_sidebar class are the sidebars affected by the bug.

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 9, 2019

Contributor

What about using a real solution like fixing the template ? col-lg flex-lg-grow-0 makes no sense, it says "hey take the space required to fill the remaining space starting from 0 width but hey... you cannot grow !" ... using col-lg-auto seems to be what was wanted here and would probably work on IE11.

This comment has been minimized.

Copy link
@stefanorigano

stefanorigano Apr 9, 2019

Contributor

The only elements in odoo using the .o_portal_sale_sidebar class are the sidebars affected by the bug.

I'm not a big fan of specific rules targeting framework classes 😕 (more down).
Website team ? @seb-odoo , @kea14
(@qsm-odoo is in holiday)


If we add (or xpath) a new element with .flex-lg-grow-0 class, it will be affected too. It's not an issue at the moment, but it will complicate things in the future.

Eg. "Client complain some elements are too big"
[...].o_portal_sale_sidebar .flex-lg-grow-0:not(btn):not(div:first-child()):not(.bg-beta) 😢

Moreover, it's probably the sidebar that should have the o_portal_sale_sidebar class (rather than the main container).
I suggest to review how classes are assigned here (solving the double classes assignment you showed me too) and then apply the fix on the right element only.

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 9, 2019

Contributor

And I am not on holiday, I am actually dying in India.

This comment has been minimized.

Copy link
@sswapnesh

sswapnesh Apr 9, 2019

Contributor

And I am not on holiday, I am actually dying in India.

I can feel the same heat. 😓

@@ -182,6 +182,12 @@ hr {
background-color: inherit;
}

// IE 11 hack
// http://browserhacks.com/#hack-d19e53a0fdfba5ec0f283ae86175a3af
*::-ms-backdrop, :root, .o_portal_sale_sidebar .flex-lg-grow-0 {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 9, 2019

Contributor

What about using a real solution like fixing the template ? col-lg flex-lg-grow-0 makes no sense, it says "hey take the space required to fill the remaining space starting from 0 width but hey... you cannot grow !" ... using col-lg-auto seems to be what was wanted here and would probably work on IE11.

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Fixing the template definitely seems the nice choice here. I wasn't aware of that bootstrap class, let me try...

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

@stefanorigano the double classes are due to the #{classes} here

<div t-attf-class="col-12 col-lg flex-lg-grow-0 #{classes}">

the variable expands to col-12 col-lg flex-lg-grow-0 d-print-none

@Julien00859 Julien00859 force-pushed the odoo-dev:12.0-opw-1944188-ie11_quotation-juc branch from 88f0717 to abdf175 Apr 9, 2019

@robodoo robodoo removed the CI 🤖 label Apr 9, 2019

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Last commit (col-lg-auto) tested and working against all browsers (IE/Edge/FF/Ch) on large and small screen.

Edit: related PR on enterprise: odoo/enterprise#4057

@Julien00859 Julien00859 requested a review from qsm-odoo Apr 9, 2019

@stefanorigano

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

and as always, @qsm-odoo saved the day 👍

@robodoo robodoo added the CI 🤖 label Apr 9, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I see col-lg flex-lg-grow-0 in enterprise too, is there a related PR ?

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

I see col-lg flex-lg-grow-0 in enterprise too, is there a related PR ?

odoo/enterprise#4057

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@qsm-odoo if it is ok for you, can you r+ ?

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@robodoo robodoo added the r+ 👌 label Apr 11, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@robodoo r- : commit message is mentioning "portal" while multiple apps are touched and I see "bellow"

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

What would a correct commit message looks like ? "and I see 'bellow'" ????

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

As you wish as long as you mention the apps you touch... I personally would write [FIX] portal, * and add other apps on first message line with * aaa, bbb, ccc.

And yes, you wrote "bellow" in your commit message...

@Julien00859 Julien00859 force-pushed the odoo-dev:12.0-opw-1944188-ie11_quotation-juc branch from abdf175 to 6640004 Apr 11, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 11, 2019

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

ping @qsm-odoo

@Julien00859

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

ping @qsm-odoo I have customers waiting

@nim-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2019

Title:

[FIX] account, portal, sale: odd sidebar on IE11

@qsm-odoo
Copy link
Contributor

left a comment

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

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

[FIX] account, portal, sale: odd sidebar on IE11
Open a quotation from the portal using IE11. The sidebar is rendered as
it did not have any width.

What we are trying to achieve on large screens is to create a sidebar
with a fix width the size of its children and to let the rest of the
content (right to the sidebar) expands. On small screens we want to
invert the direction from left-to-right to up-to-bottom.

The classes `col-12`, `col-lg` and `flex-lg-grow-0` are used to fulfill
that purpose.

The problem is due to the rule `flex-grow: 0 !important` applied by the
selector `.flex-lg-grow-0` on the sidebar. On Chrome/Firefox the dom
element takes the width of his children as minimal width, the
`flex-grow: 0` does not shrink the element below that minimal width.
IE11 does not set a minimal width based on the child elements of the
node, the width is equal to 0 and the `flex-grow: 0` forbid it from
growing.

The solution is to replace the said classes by the `col-lg-auto` class
that does exactly what we want and is cross-browser.

opw-1944188
opw-1935087

@nim-odoo nim-odoo force-pushed the odoo-dev:12.0-opw-1944188-ie11_quotation-juc branch from 6640004 to d37948b Apr 16, 2019

@nim-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

robodoo r+

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Merged, thanks!

@Julien00859 Julien00859 deleted the odoo-dev:12.0-opw-1944188-ie11_quotation-juc branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.