[FIX] web_responsive: Hide footer instead of remove #446

lasley · 2016-10-15T13:37:34Z

Add style to hide oe_footer instead of remove to not break support_branding
Add note in ReadMe explaining override of support_branding
Add oe_main_menu_navbar id to navbar

This is the v9 fix for support_branding in #445

pedrobaeza · 2016-10-15T16:57:45Z

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?

lasley · 2016-10-15T17:03:09Z

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.

pedrobaeza · 2016-10-15T17:08:04Z

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?

lasley · 2016-10-15T17:08:12Z

Regarding the company logo - I can add in the title pane of the app drawer in v9, but it shrunk considerably in v10 and I don't think would be viable:

vs

I admittedly like the smaller UI of 10, so I hesitate to change. Is the missing logo something that will bug people you think?

lasley · 2016-10-15T17:10:01Z

It's pretty heavy. If I leave them, the entire menus are duplicated. The interesting thing about the way the menus are generated too is that every menu is added, but hidden. This entire section would be duplicated - 4.5k lines of code on my local:

pedrobaeza · 2016-10-15T17:11:09Z

There are 2 cons for not having logo:

The company branding, that all company managers wants to have in their systems (well, everywhere)
In multi-company environments, users look at the logo to see in which company they are.

pedrobaeza · 2016-10-15T17:12:00Z

OK, let's see what we get with the replacings. Don't touch that part for now.

pedrobaeza · 2016-10-15T17:13:18Z

Even smaller, in v10 the logo can be there as something testimonial (or include a system parameter to configure the height at request)

lasley · 2016-10-15T17:13:47Z

It shouldn't be too difficult to add the logo in the bottom center of the App Drawer. Will that work?

pedrobaeza · 2016-10-15T17:15:01Z

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.

lasley · 2016-10-15T17:20:10Z

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).

pedrobaeza · 2016-10-15T17:51:09Z

Isn't better to put the logo inside the top bar instead of overlapping the apps icons?

lasley · 2016-10-15T17:52:44Z

Yeah that was the intent, and what it looked like on my local - This definitely wasn't what I was going for 😆

pedrobaeza · 2016-10-15T17:53:33Z

hehe, we definitively need that docker that mimicks Runbot behavior on local 😉

lasley · 2016-10-15T17:56:54Z

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.

lasley · 2016-10-15T18:18:09Z

Rebased for Travis changes so my Runbot doesn't skip the builds anymore. The last commit should hopefully fix the logo issue too

lasley · 2016-10-16T17:13:57Z

Note to self, looks like junk on mobile

lasley · 2016-10-16T19:21:40Z

Annnnnd now it doesn't! This is ready for review again I believe.

Note that this is still incompatible with the support_branding module as detailed in the ReadMe (build isn't broken, but support_branding assets are simply hidden). A glue module will need to be created in order for the support_branding stuff to be injected somewhere other than web.secondary_menu, which is the top bar in this interface.

pedrobaeza · 2016-10-16T19:25:35Z

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.

lasley · 2016-10-16T19:42:08Z

Alright updated the ReadMe. Logo sizing is better now too

lasley · 2016-10-16T19:43:42Z

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

pedrobaeza · 2016-10-16T20:18:05Z

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".

yajo · 2016-10-17T09:26:00Z

web_responsive/views/web.xml

@@ -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>


Better add class hidden if possible.

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.

Then why not just add .oe_footer {display:none} to the Less file?

Because I didn't think of it? 😆 👍

yajo · 2016-10-17T09:26:57Z

web_responsive/views/web.xml

+                        </h4>
+                    </div>
+                    <div class="col-xs-6">
+                        <a class="oe_logo pull-right" t-att-href="'/web/?debug' if debug else '/web'">


Can't you use keep_query?

yajo · 2016-10-17T09:27:42Z

web_responsive/views/web.xml

+                    <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" />


You'll probably get dirty translations with this. Better use a div for fontawesome icons or add t-translation="off" please.

lasley · 2016-10-18T18:59:29Z

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 oe_main_menu_navbar id which triggered some new CSS. Looks more like the v10 branch now, which makes me happy.

pedrobaeza · 2016-10-18T19:43:32Z

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.

lasley · 2016-10-18T20:17:28Z

Fixed - brought in the v10 style, just nested differently

pedrobaeza · 2016-10-18T20:51:45Z

Uhm, entering as demo user, I don't see any app nor menu...

lasley · 2016-10-18T21:01:44Z

Should demo user have access to Settings or Apps menus? I installed a few apps and it seems to work fine.

lasley · 2016-10-18T21:03:56Z

Taking it a step further, I added Settings to Demo User Admin access rights & answered my own question. Demo user does not have access to the initial 2 app icons - but can see them once the change is made:

pedrobaeza · 2016-10-18T21:15:12Z

But at least "Discuss" app should be available I think...

lasley · 2016-10-18T21:34:05Z

Nope - discuss is an additional install it seems. This is a screencap from a fresh installed DB:

pedrobaeza

Thanks for checking.

oscarolar

Thanks!

pedrobaeza · 2016-10-19T05:48:37Z

@yajo, please check

yajo

Enter mobile mode as demo user and choose "Sales". When the submenu is hidden, you see this:

using firefox

lasley · 2016-10-19T14:22:43Z

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.

pedrobaeza · 2016-10-19T16:22:01Z

Waiting for @yajo's confirmation to merge.

lasley · 2016-10-19T16:23:39Z

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 😄

yajo

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`

lasley · 2016-10-20T15:47:51Z

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 🍏

lasley added bug needs review labels Oct 15, 2016

lasley added this to the 9.0 milestone Oct 15, 2016

lasley mentioned this pull request Oct 15, 2016

[MIG][10.0] web_responsive #443

Merged

1 task

lasley force-pushed the hotfix/9.0/web_responsive-support_branding branch from a779427 to cec6def Compare October 15, 2016 13:59

lasley force-pushed the hotfix/9.0/web_responsive-support_branding branch from 694dea9 to df5b1e4 Compare October 15, 2016 18:16

lasley force-pushed the hotfix/9.0/web_responsive-support_branding branch from 6364817 to 987a788 Compare October 15, 2016 18:54

yajo requested changes Oct 17, 2016

View reviewed changes

lasley force-pushed the hotfix/9.0/web_responsive-support_branding branch from 93342c0 to 3d6d45a Compare October 17, 2016 21:44

lasley force-pushed the hotfix/9.0/web_responsive-support_branding branch 2 times, most recently from d39dc0a to 26c3647 Compare October 18, 2016 20:16

pedrobaeza approved these changes Oct 18, 2016

View reviewed changes

oscarolar approved these changes Oct 19, 2016

View reviewed changes

yajo requested changes Oct 19, 2016

View reviewed changes

lasley mentioned this pull request Oct 19, 2016

[MIG][8.0] web_responsive #445

Closed

lasley force-pushed the hotfix/9.0/web_responsive-support_branding branch from ec275e2 to f74af2a Compare October 19, 2016 14:22

yajo approved these changes Oct 20, 2016

View reviewed changes

lasley force-pushed the hotfix/9.0/web_responsive-support_branding branch from 7b1b300 to fb29a53 Compare October 20, 2016 15:46

pedrobaeza merged commit e3b5d73 into OCA:9.0 Oct 20, 2016

lasley deleted the hotfix/9.0/web_responsive-support_branding branch October 20, 2016 16:32

[FIX] web_responsive: Hide footer instead of remove #446

[FIX] web_responsive: Hide footer instead of remove #446

Conversation

lasley commented Oct 15, 2016

pedrobaeza commented Oct 15, 2016

lasley commented Oct 15, 2016

pedrobaeza commented Oct 15, 2016

lasley commented Oct 15, 2016

lasley commented Oct 15, 2016 • edited Loading

pedrobaeza commented Oct 15, 2016

pedrobaeza commented Oct 15, 2016

pedrobaeza commented Oct 15, 2016

lasley commented Oct 15, 2016

pedrobaeza commented Oct 15, 2016

lasley commented Oct 15, 2016

pedrobaeza commented Oct 15, 2016

lasley commented Oct 15, 2016

pedrobaeza commented Oct 15, 2016

lasley commented Oct 15, 2016

lasley commented Oct 15, 2016 • edited Loading

lasley commented Oct 16, 2016

lasley commented Oct 16, 2016

pedrobaeza commented Oct 16, 2016

lasley commented Oct 16, 2016

lasley commented Oct 16, 2016

pedrobaeza commented Oct 16, 2016

yajo Oct 17, 2016

Choose a reason for hiding this comment

lasley Oct 17, 2016

Choose a reason for hiding this comment

yajo Oct 18, 2016

Choose a reason for hiding this comment

lasley Oct 18, 2016

Choose a reason for hiding this comment

yajo Oct 17, 2016

Choose a reason for hiding this comment

yajo Oct 17, 2016

Choose a reason for hiding this comment

lasley commented Oct 18, 2016 • edited Loading

pedrobaeza commented Oct 18, 2016

lasley commented Oct 18, 2016 • edited Loading

pedrobaeza commented Oct 18, 2016

lasley commented Oct 18, 2016 • edited Loading

lasley commented Oct 18, 2016 • edited Loading

pedrobaeza commented Oct 18, 2016

lasley commented Oct 18, 2016

pedrobaeza left a comment

Choose a reason for hiding this comment

oscarolar left a comment

Choose a reason for hiding this comment

pedrobaeza commented Oct 19, 2016

yajo left a comment

Choose a reason for hiding this comment

lasley commented Oct 19, 2016 • edited Loading

pedrobaeza commented Oct 19, 2016

lasley commented Oct 19, 2016 • edited Loading

yajo left a comment

Choose a reason for hiding this comment

lasley commented Oct 20, 2016 • edited Loading

lasley commented Oct 15, 2016 •

edited

Loading

lasley commented Oct 15, 2016 •

edited

Loading

lasley commented Oct 18, 2016 •

edited

Loading

lasley commented Oct 18, 2016 •

edited

Loading

lasley commented Oct 18, 2016 •

edited

Loading

lasley commented Oct 18, 2016 •

edited

Loading

lasley commented Oct 19, 2016 •

edited

Loading

lasley commented Oct 19, 2016 •

edited

Loading

lasley commented Oct 20, 2016 •

edited

Loading