-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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_responsive: Hide footer instead of remove #446
[FIX] web_responsive: Hide footer instead of remove #446
Conversation
a779427
to
cec6def
Compare
We should take the occasion to hide all replaced elements instead of just removing them, so we maximize the compatibility with other possible modules. One thing: what about putting the company logo a bit more smaller in the upper right corner of the drawer? |
I originally hid everything, but was told by @yajo to remove instead in the following comments:
So I should revert all that? I agree with his logic TBH - there's a lot of JS happening & menus getting filled in that adds a lot of load to the page without replacing. |
When there's JS logic involved, I agree on the optimization principle, but we don't know if other modules will mess up with these removals. Is it so heavy to let them? |
There are 2 cons for not having logo:
|
OK, let's see what we get with the replacings. Don't touch that part for now. |
Even smaller, in v10 the logo can be there as something testimonial (or include a system parameter to configure the height at request) |
It shouldn't be too difficult to add the logo in the bottom center of the App Drawer. Will that work? |
Well, if we put it with absolute positioning plus float feature, it would be enough, but it's useless if the logo is not shown when opening because you have to scroll down. |
Agreed. I just looked at Enterprise for some possible inspiration, but it seems they put it at the bottom and required scroll. I can increase the height of the App Drawer title bar in 10 to support it at the top then. I can't really think of a better spot for it, and there is space there. I am planning on adding a Search Bar there too, but that can go top middle. Or maybe the logo top middle and search top right. I don't think a param will work because the height is controlled in CSS, so I wouldn't be able to grab the param (at least I don't think so, let me know if I'm missing some Odoo magic). |
Isn't better to put the logo inside the top bar instead of overlapping the apps icons? |
Yeah that was the intent, and what it looked like on my local - This definitely wasn't what I was going for 😆 |
hehe, we definitively need that docker that mimicks Runbot behavior on local 😉 |
Seriously - my dev environment situation sucks at the moment. This is the first I've bothered with the other Odoo versions, which turns out to be a real pain. Something will be figured out, sooner rather than later - that's for sure. My poor juniors will be so 😕 with this workflow. |
694dea9
to
df5b1e4
Compare
Rebased for Travis changes so my Runbot doesn't skip the builds anymore. The last commit should hopefully fix the logo issue too |
6364817
to
987a788
Compare
Annnnnd now it doesn't! This is ready for review again I believe. Note that this is still incompatible with the |
Thanks for the efforts, Dave. I have tried and I feel that the logo is a bit small, leaving a big margin on top and bottom. Or is this to be the same for v10? About support_branding, we can remove the word incompatible, because as said it doesn't fail, but one of the features isn't simply shown. But the other is still there: the error dialog includes the possibility of sending the traceback to a support mail. |
Dang now that I see the ruler guides against the "Apps" link, I realize that the Apps link is not center. That's going to bug me now that I know about it, so will fix |
This looks much better, but I'm afraid there's still one thing to do: restore the "Edit company data" link. In v9, without it, you can't edit company data, unless you activate "Multi companies". |
@@ -102,7 103,9 @@ | |||
|
|||
<xpath expr="//a[@class='oe_logo']" position="replace" /> | |||
|
|||
<xpath expr="//div[@class='oe_footer']" position="replace" /> | |||
<xpath expr="//div[@class='oe_footer']" position="attributes"> | |||
<attribute name="style">display: none;</attribute> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better add class hidden
if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to append into the existing class? The goal here is to allow compatibility with addons that may target the div. At the moment it's oe_footer
, but I don't know if other modules inject classes into that or other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why not just add .oe_footer {display:none}
to the Less file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I didn't think of it? 😆 👍
</h4> | ||
</div> | ||
<div class="col-xs-6"> | ||
<a class="oe_logo pull-right" t-att-href="'/web/?debug' if debug else '/web'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you use keep_query
?
<div class="col-xs-6"> | ||
<h4 class="app-drawer-panel-title pull-left"> | ||
<a href="#" class="app-drawer-icon-close drawer-toggle"> | ||
<i class="fa fa-lg fa-chevron-left" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll probably get dirty translations with this. Better use a div for fontawesome icons or add t-translation="off"
please.
93342c0
to
3d6d45a
Compare
Alright we should be good. I added a bonus fix on the toggle button padding/border too 😄 Note the padding had to be made a bit smaller due to adding of |
Very good! Just one minor thing: edit icon shouldn't be visible when you're not admin, as you don't have rights to edit company data. |
d39dc0a
to
26c3647
Compare
Fixed - brought in the v10 style, just nested differently |
Uhm, entering as demo user, I don't see any app nor menu... |
But at least "Discuss" app should be available I think... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@yajo, please check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ec275e2
to
f74af2a
Compare
Well that's sad - I have to fully remove the oe_main_menu_navbar id in order to stop a bunch of the core JS from firing. This resolved the @Yajo comment I don't see an easy way around this & there wasn't a particular reason for me doing it outside of simply wanting the previous ID to exist for other modules that may target it, so I removed & reverted the styling fixes associated with it. |
Waiting for @yajo's confirmation to merge. |
Once he confirms, I would like to squash the commits logically as well in order to allow for easier cherry-picking into the forward/backports & to not conflict with our OCD for a clean diff 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's sad that you had to remove that, but this now works, so it's OK.
Maybe you could add that to known issues if you want, but it's OK anyway.
Waiting until you rebase to merge.
* Add style to hide oe_footer instead of remove to not break `support_branding` * Add company logo and editing to App Drawer title bar * Remove translation and screen reader from glyphs * Minor style fixes on app drawer toggle button positioning * Add note in ReadMe explaining override of `support_branding` * Add note in ReadMe explaining why no `oe_main_menu_navbar`
7b1b300
to
fb29a53
Compare
I agree that it's sad I had to remove it - I was pretty excited when I added it, particularly when I noticed the styling differences. Adding to known issues is a good call, I can see that being trying again later. Done. Rebase is done as well, turns out the logical commit was simply the one. Was worth the check though IMO - I believe we're good for merge once we get CI 🍏 |
support_branding
support_branding
oe_main_menu_navbar
id to navbarThis is the v9 fix for
support_branding
in #445