[IMP][11.0][web_responsive] Add option to select chatter position #951

Tardo · 2018-06-11T14:02:58Z

Add option in user settings to select chatter position.

Usage:

Go to user preferences and select "Chatter Position"

yajo · 2018-06-12T09:30:14Z

web_chatter_right/README.rst

+WEB CHATTER RIGHT
+=================
+
+Put chatter right


Please use new split readme system

yajo · 2018-06-12T09:30:29Z

web_chatter_right/__init__.py

+#    You should have received a copy of the GNU General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+##############################################################################


Remove all of this.

yajo · 2018-06-12T09:30:49Z

web_chatter_right/__manifest__.py

+#    You should have received a copy of the GNU General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+##############################################################################


🔥 Use short license headers, as found in the official template.

yajo · 2018-06-12T09:31:00Z

web_chatter_right/__manifest__.py

+    'name': 'Web Chatter Right',
+    'version': '11.0.1.0.0',
+    'author': "Odoo Community Association (OCA), Alexandre Díaz <dev@redneboa.es>",
+    'website': '',


Put oca repo website

yajo · 2018-06-12T09:31:10Z

web_chatter_right/__manifest__.py

+    ],
+    'qweb': [],
+    'test': [
+    ],


Remove empty keys

yajo · 2018-06-12T09:32:06Z

web_chatter_right/static/src/css/chatter_right.css

+  .oe_chatter.o_chatter_right {
+    display: inline-block !important;
+    width: 390px !important;
+    min-width: 390px !important;


Do not use !important. Those rules are extremely hard to theme. Better, use more specific selectors if you need to override some odoo css.

Also, use Less if possible.

yajo · 2018-06-12T09:32:12Z

web_chatter_right/static/src/css/chatter_right.css

+  }
+
+  .o_web_client > .o_main > .o_main_content > .o_content .table-responsive {
+    overflow-x: auto !important;


yajo · 2018-06-12T09:32:25Z

web_chatter_right/static/src/js/chatter_right.js

+   * Theme Chatter Right
+   * GNU Public License
+   * Alexandre Díaz <dev@redneboa.es>
+   */


Use short headers as in template.

yajo · 2018-06-12T09:34:52Z

web_chatter_right/static/src/js/chatter_right.js

@@ -0,0 +1,30 @@
+odoo.define('web_chatter_right.Chatter', function (require) {


2 options:

a. call it web_chatter_side. Arabic readers may want it left.

b. Do not use a new addon. Instead, add the features to web_responsive. (I prefer this one)

yajo · 2018-06-12T09:35:49Z

web_chatter_right/static/src/js/chatter_right.js

+          var $chatter = this.$el.find('.oe_chatter');
+          if (this.has_sheet && $chatter.length !== 0) {
+              if ($chatter.css('visibility') === 'hidden' || $chatter.css('display') === 'none') {
+                  this.$el.find('.o_form_sheet_bg').removeClass("o_form_sheet_chatter_right");


Possibly you could do all of this overriding the XML template instead, and it would be more clear.

Also, users should be able to choose these settings.

yajo

Some comments on code below. But it's awesome!

I've been comparing it with how web_enterprise makes it, and I noticed a couple of little UI/UX details that would be IMHO important:

Add some separation between the form and the chatter. Just 1px line, like in the picture:
Separate scroll bars for form and chatter. With your patch, user scrolls both or none, but separate scrolling is useful:

yajo · 2018-06-13T08:48:47Z

web_responsive/readme/DESCRIPTION.rst

+==============
+Web Responsive
+==============
+


Remove all until here. The new split readme system generates all this boilerplate.

yajo · 2018-06-13T08:49:16Z

web_responsive/readme/DESCRIPTION.rst

+ * Keyboard shortcuts for easier navigation
+ * Display kanban views for small screens if an action or field One2x
+ * Set chatter side (Optional per user)
+


Remove from here...

yajo · 2018-06-13T08:49:24Z

web_responsive/readme/DESCRIPTION.rst

+
+Usage
+=====
+


... until here.

yajo · 2018-06-13T08:49:41Z

web_responsive/readme/DESCRIPTION.rst

+Usage
+=====
+
+Keyboard Shortcuts


Move this section to readme/USAGE.rst

yajo · 2018-06-13T08:49:58Z

web_responsive/readme/DESCRIPTION.rst

+* Navigate Apps Drawer - Arrow Keys
+* Type to select App Links
+* ``esc`` to close App Drawer
+


Remove from here to the end.

yajo · 2018-06-13T09:08:02Z

web_responsive/static/src/js/web_responsive.js

+            var self = this;
+            var $chatter = this.$el.find('.oe_chatter');
+            if (this.has_sheet && $chatter.length !== 0) {
+                Rpc.query({


This is not the good hook to call for this. You're asking for the setting before you render each form. A normal user session renders many forms, so we should try to save requests whenever possible, as they affect directly to the performance. You have 2 options, to avoid doing so many calls:

Call once, cache request. Similar to what's done in [FIX] web_dialog_size: Fix usage for non-admins and reduce calls #954.

Manage this from the backend. Overwrite template web.webclient_bootstrap, by adding a class such as chatter-position-#{user.chatter_position}.

Then, no JS is needed at all; you can reposition it through Less directly.

IMHO I'd go for 2nd option. I think it will be faster for the user and easier to maintain. But you choose 😉

yajo · 2018-06-13T09:10:28Z

web_responsive/views/inherited_view_users_form_simple_modif.xml

+        <field name="chatter_position" />
+      </xpath>
+    </field>
+  </record>


Please use 4 spaces for indentation, to be consistent with other files.

yajo · 2018-06-19T07:21:40Z

web_responsive/__init__.py

@@ -1,4 +1,4 @@
-# Copyright 2018 Tecnotel - Alexandre Díaz
+# Copyright 2018  Alexandre Díaz


yajo · 2018-06-19T07:22:09Z

web_responsive/models/__init__.py

 # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).

 from . import inherited_res_users
+from . import inherited_ir_http


No need to prefix with inherited_

yajo · 2018-06-19T07:22:30Z

web_responsive/models/inherited_res_users.py

@@ -1,4 +1,4 @@
-# Copyright 2018 Tecnotel - Alexandre Díaz
+# Copyright 2018  Alexandre Díaz


yajo · 2018-06-19T07:23:02Z

web_responsive/readme/USAGE.rst

@@ -0,0 +1,13 @@
+Keyboard Shortcuts
+------------------


Remove this header please.

yajo · 2018-06-19T07:23:14Z

web_responsive/readme/USAGE.rst

+
+.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
+   :alt: Try me on Runbot
+   :target: https://runbot.odoo-community.org/runbot/162/11.0


Remove the runbot stuff please. It should be autofilled.

yajo · 2018-06-19T07:28:17Z

web_responsive/static/src/js/web_responsive.js

+        _updateView: function ($newContent) {
+            this._super($newContent);
+            var $chatter = this.$el.find('.oe_chatter');
+            if ($chatter.length === 0 || $chatter.css('visibility') === 'hidden' || $chatter.css('display') === 'none') {


Use $chatter.is(":hidden")

yajo · 2018-06-19T07:30:35Z

web_responsive/static/src/js/web_responsive.js

+            if ($chatter.length === 0 || $chatter.css('visibility') === 'hidden' || $chatter.css('display') === 'none') {
+                this.$el.find('.o_form_sheet_bg').removeClass("o_form_sheet_bg_chatter_visible");
+            } else {
+                this.$el.find('.o_form_sheet_bg').addClass("o_form_sheet_bg_chatter_visible");


I'm wondering if do we need all of this now. 🤔

You are now adding a class to the whole web client that indicates the chatter position. Can't we use that class directly in Less to change the chatter position, and remove all related JS code?

I see that the point is to have 100% width in forms without chatter, but see suggestions below on Less code.

yajo · 2018-06-19T07:32:27Z

web_responsive/views/web.xml

@@ -94,6 +94,10 @@

        </xpath>

+        <xpath expr="//div[@class='o_main']" position="attributes">


Use the v11 new hasclass() method instead.

yajo · 2018-06-19T07:57:17Z

web_responsive/static/src/less/form_view.less

 * License LGPL-3.0 or later (http://www.gnu.org/licenses/lgpl). */

+@sheet-margin: @sheet-padding;
+@chatter-side-width: 390px;


I'd use some variable from current bootstrap theme. i.e. @screen-md / 3

yajo · 2018-06-19T08:18:36Z

web_responsive/static/src/less/form_view.less

+@chatter-side-width: 390px;
+
+// Sided Chatter
+.o_chatter_position_sided {


My suggestion is to use flexbox here. You have a playground in https://demo.agektmr.com/flexbox/. If I'm not wrong, this should do the trick (or almost):

@media (min-width: @screen-md) { .o_chatter_position_sided { .o_form_view { display: flex; flex-flow: row; height: 100%; align-items: stretch; .o_form_sheet_bg { border-right: 1px solid @table-border-color; overflow: auto; flex: 3 0 auto; } .o_chatter { overflow: auto; flex: 1 1 @chatter-side-width; .o_chatter_topbar { height: auto; flex-wrap: wrap; } } } } }

The benefits:

No JS needed. Thus, more fast.

Adaptable to any size.

Autofills if no chatter available.

Cleaner, IMHO.

Tardo · 2018-06-19T13:55:50Z

Thanks for all the help @yajo , now i know how flex property works xD and like chatter toolbar solution too!

yajo

Code looks perfect, and functionality too. Thanks for the hard job!

The Travis ❌ is due to new linters that affect other files in the addon, so the fault is not yours. If you want to fix it, that'd be OK, but not a requirement IMHO.

elicoidal · 2018-06-20T07:27:50Z

I would suggest to keep travis green otherwise it will pollute the repo

pedrobaeza · 2018-06-20T08:03:02Z

I have cleaned the branch and rebase this one, so it should be all correct now.

pedrobaeza · 2018-06-20T08:52:22Z

Good work with this, @Tardo !

Using the new feature, I see that it's better than enterprise one, not using too much width, but sometimes this can be short for showing large content, like embedded large images. Do you think it's possible to make the width configurable through the typical arrows?

Another thing I have seen. With this width, not everything fits on the first line:

making a visual inconsistency, not having connected the opened tab with the rest of the content, which is very important IMO for identifying easily if you are sending an internal message or a public one. See the difference with the right normal one:

Possible solutions to this are:

Make the width bigger, but this depends on the translation of the elements.
Put the followers up. My colleague @yajo said to me that it's very easy with flexbox.

pedrobaeza · 2018-06-20T08:55:48Z

Ah, another thing abusing a bit 👼

On previous versions, if the chatter in bottom, the content was fit to 100%. Is it possible that you change also this thing in this PR?

Thanks again.

yajo

With the changes I suggest below, you get this behavior, automatically, depending on user screen width:

yajo · 2018-06-20T08:54:47Z

web_responsive/static/src/less/form_view.less

+            .o_form_sheet_bg {
+                border-right: 1px solid @table-border-color;
+                overflow: auto;
+                flex: 1 1 auto;


3 0 auto, which means: give him 3/4 of the space when growing, but not let it shrink less than 1/2 of the space

Use different grow & shrink values for me makes the view "inconsistent" ...

Well, the point is that i.e. here I'm working with an ultrawide monitor. Under this circumstance, when window is full width, the chatter is extremely small. Emails sometimes have fixed widths, and with such a big monitor, it seems more comfortable to make it grow a little bit when the screen is big enough. Instead of making it feel unconsistent, here it feels more responsive.

However, if you prefer, you can combine flex with min-width and max-width. Play with those as you like. For instance, setting the chatter as 1/3 of the window, with min-width: @chatter-side-width. Do you prefer if I add the commit myself, since I have this monitor to test with?

Ok, i changed width to use percentage instead fixed width...
Feel free to commit changes what you think appropriate.

yajo · 2018-06-20T08:56:54Z

web_responsive/static/src/less/form_view.less

+
+            .o_chatter {
+                overflow: auto;
+                flex: 0 0 @chatter-side-width;


1 1 @chatter-side-width, which means: give him 1/4 of the space when growing, also when shrinking, and start with @chatter-side-width to make computations.

yajo · 2018-06-20T09:04:21Z

web_responsive/static/src/less/form_view.less

+                    flex-wrap: wrap;
+                }
+            }
+            .oe_chatter:empty {


You can put this inside the .o_chatter block and use &:empty instead.

The element only have the class "oe_chatter" when it's empty ... perhaps best use oe_chatter for all?

Oh, I didn't know that. Then maybe yes, we can use oe_chatter for both.

yajo · 2018-06-20T09:06:21Z

web_responsive/static/src/less/form_view.less

+                .o_chatter_topbar {
+                    height: auto;
+                    flex-wrap: wrap;
+                }


Inside this block, add this other one:

.o_followers { order: -10; flex: 2 0 @chatter-side-width; }

That means: Put this element before the other ones (that default to order: 0); then, give him the double than the other ones when growing (because it has 2 buttons), but do not let it shrink smaller than @chatter-side-width

I have better results (if chatter have fixed size) with:

.o_followers { order: -10; flex: 0 1 @chatter-side-width; }

Use all the chatter width (no more, but if needed less)

OK, seems legit.

yajo · 2018-06-22T11:27:08Z

web_responsive/static/src/js/web_responsive.js

    var ViewManager = require('web.ViewManager');
+    var Chattter = require('mail.Chatter');


If you do this, you must depend on mail. Check runbot, it's not loading this file.

yajo · 2018-06-22T11:38:59Z

web_responsive/static/src/js/web_responsive.js

+            var self = ev.data;
+            if (self._mousePosX !== -1) {
+                var wDiff = self._mousePosX - ev.pageX;
+                self.$el.css('flex-basis', self._origWidth + wDiff + 'px');


I'm not sure if I prefer you to add the dependency on mail, or better just remove this feature. I think this feature is too complex and nobody will care about resizing that column if it has a sane and properly responsive default.

You know, the famous debate between configurability vs. sane defaults... I think this is a good case for sane defaults. If you need to see wider mails, the officially supported answer should be: "configure your chatter in the bottom; it will have full width", don't you think?

Besides, by reading the code I'd say you are not storing this anywhere... So each time I press F5 I lose my custom width? And if I resize the window up and down, do I have to be dragging it all the time? It seems one of those features that nobody will care if it's not present, but yes if it's incomplete.

Just some thoughts, please don't take this badly. 😇

If we have to add the dependency, I agree it's better to stick to a sane fixed width that can vary according screen size.

If you agree, since you originated the request, I'm gonna remove this. Let's see how it works after that.

pedrobaeza · 2018-06-22T16:19:16Z

OK, I see it now perfect! @Tardo do you agree also? Should we merge?

Do you have Twitter for announcing the improvement there?

Tardo · 2018-06-22T17:49:47Z

For me all ok to merge. Yes i have an twitter account.

pedrobaeza · 2018-06-22T17:55:32Z

Thanks for the work. Please tell me which one for mentioning you for giving some way thanks.

Tardo · 2018-06-22T18:07:04Z

My twitter account is: NoMoreButtons ;)

LRovira-PlanetaTIC · 2018-06-25T08:11:17Z

The preferences option is only available for users with administration rights. It is right?

yajo · 2018-06-25T08:14:20Z

Yikes, true, that's a bug we didn't find at time for release:

…OCA#951)

yajo added the work in progress label Jun 12, 2018

yajo added this to the 11.0 milestone Jun 12, 2018

yajo requested changes Jun 12, 2018

View reviewed changes

Tardo changed the title ~~[ADD][11.0] web_chatter_right~~ [IMP][11.0][web_responsive] Add option to select chatter position Jun 12, 2018

yajo requested changes Jun 13, 2018

View reviewed changes

yajo requested changes Jun 19, 2018

View reviewed changes

yajo approved these changes Jun 20, 2018

View reviewed changes

pedrobaeza force-pushed the web_chatter_right branch from 2de0fb6 to f5729e4 Compare June 20, 2018 08:02

pedrobaeza removed the work in progress label Jun 20, 2018

yajo requested changes Jun 20, 2018

View reviewed changes

yajo requested changes Jun 22, 2018

View reviewed changes

yajo reviewed Jun 22, 2018

View reviewed changes

[ADD][11.0] web_chatte_right

ab24dcf

pedrobaeza force-pushed the web_chatter_right branch from 7a6f7b0 to ab24dcf Compare June 22, 2018 16:22

pedrobaeza merged commit e5e109e into OCA:11.0 Jun 22, 2018

Tardo mentioned this pull request Jun 25, 2018

[FIX][11.0][web_responsive] Chatter Position #966

Merged

Tardo deleted the web_chatter_right branch July 3, 2018 17:34

yajo pushed a commit to Tecnativa/web that referenced this pull request Oct 5, 2018

[IMP] web_responsive: Possibility of putting the chatter in the right (…

85a95c3

…OCA#951)

yajo pushed a commit to Tecnativa/web that referenced this pull request Dec 10, 2018

[IMP] web_responsive: Possibility of putting the chatter in the right (…

53d9a6e

…OCA#951)

tschanzt pushed a commit to camptocamp/web that referenced this pull request Jun 19, 2019

[IMP] web_responsive: Possibility of putting the chatter in the right (…

2d8fe8e

…OCA#951)

Tardo added a commit to Tecnativa/web that referenced this pull request Sep 30, 2019

[IMP] web_responsive: Possibility of putting the chatter in the right (…

1efa992

…OCA#951)

Tardo added a commit to Tecnativa/web that referenced this pull request Oct 13, 2019

[IMP] web_responsive: Possibility of putting the chatter in the right (…

b876a7d

…OCA#951)

pierre-pizzetta pushed a commit to pierre-pizzetta/web that referenced this pull request Oct 17, 2020

[IMP] web_responsive: Possibility of putting the chatter in the right (…

96cda51

…OCA#951)

SplashS pushed a commit to SplashS/web that referenced this pull request Feb 11, 2021

[IMP] web_responsive: Possibility of putting the chatter in the right (…

82ecce7

…OCA#951)

SplashS pushed a commit to SplashS/web that referenced this pull request Mar 17, 2021

[IMP] web_responsive: Possibility of putting the chatter in the right (…

23bddb1

…OCA#951)

SplashS pushed a commit to SplashS/web that referenced this pull request Oct 21, 2021

[IMP] web_responsive: Possibility of putting the chatter in the right (…

c1e8128

…OCA#951)

SplashS pushed a commit to SplashS/web that referenced this pull request Nov 15, 2021

[IMP] web_responsive: Possibility of putting the chatter in the right (…

e00d615

…OCA#951)

bvanjeelharia pushed a commit to bvanjeelharia/web that referenced this pull request Feb 6, 2023

[IMP] web_responsive: Possibility of putting the chatter in the right (…

3b5cd6f

…OCA#951)

bvanjeelharia pushed a commit to bvanjeelharia/web that referenced this pull request Feb 6, 2023

[IMP] web_responsive: Possibility of putting the chatter in the right (…

9aaf772

…OCA#951)

bvanjeelharia pushed a commit to bvanjeelharia/web that referenced this pull request Feb 7, 2023

[IMP] web_responsive: Possibility of putting the chatter in the right (…

242c222

…OCA#951)

bvanjeelharia pushed a commit to bvanjeelharia/web that referenced this pull request Mar 1, 2023

[IMP] web_responsive: Possibility of putting the chatter in the right (…

7022039

…OCA#951)

TesaLazarenko pushed a commit to TesaLazarenko/oca_web that referenced this pull request Nov 17, 2023

[IMP] web_responsive: Possibility of putting the chatter in the right (…

9817f31

…OCA#951)

davidtranhp pushed a commit to davidtranhp/web that referenced this pull request Feb 7, 2024

[IMP] web_responsive: Possibility of putting the chatter in the right (…

22c77dd

…OCA#951)

davidtranhp pushed a commit to davidtranhp/web that referenced this pull request Feb 7, 2024

[IMP] web_responsive: Possibility of putting the chatter in the right (…

e171d98

…OCA#951)

		@@ -0,0 +1,30 @@
		odoo.define('web_chatter_right.Chatter', function (require) {

		@@ -1,4 +1,4 @@
		# Copyright 2018 Tecnotel - Alexandre Díaz
		# Copyright 2018 Alexandre Díaz

		@@ -94,6 +94,10 @@

		</xpath>

		<xpath expr="//div[@class='o_main']" position="attributes">

		var ViewManager = require('web.ViewManager');
		var Chattter = require('mail.Chatter');


		Usage
		=====

[IMP][11.0][web_responsive] Add option to select chatter position #951

[IMP][11.0][web_responsive] Add option to select chatter position #951

Conversation

Tardo commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tardo commented Jun 19, 2018

yajo left a comment

Choose a reason for hiding this comment

elicoidal commented Jun 20, 2018

pedrobaeza commented Jun 20, 2018

pedrobaeza commented Jun 20, 2018

pedrobaeza commented Jun 20, 2018

yajo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 22, 2018

Tardo commented Jun 22, 2018

pedrobaeza commented Jun 22, 2018

Tardo commented Jun 22, 2018

LRovira-PlanetaTIC commented Jun 25, 2018

yajo commented Jun 25, 2018

Tardo commented Jun 11, 2018 •

edited

Loading

yajo left a comment •

edited

Loading