Refactoring of the rendering of views' buttons #23115

qsm-odoo · 2018-02-16T12:01:22Z

Code refactoring and removal of oe_highlight.
See enterprise branch: https://github.com/odoo/enterprise/pull/1878

qsm-odoo · 2018-02-16T12:03:16Z

addons/web/static/src/js/views/form/form_renderer.js

-                .text(node.attrs.string)
-                .appendTo($button);
-        }
+        var $button = this._renderButtonFromNode(node); // FIXME force oe_stat_button ?


The previous code forced the 'oe_stat_button' class, the new code supposes it is explicitly set in the view... is this a problem ?

maybe... this should probably not even appear in views' arch, and should be automatically added by the webclient

qsm-odoo · 2018-02-16T12:07:26Z

@ged-odoo I let you assign as reviewer whoever is motivated 😄

pedrobaeza · 2018-02-16T12:14:26Z

@qsm-odoo does this mean that oe_highlight in community modules is not going to work anymore for v12 and should be replaced by btn-primary?

qsm-odoo · 2018-02-16T12:26:20Z

@pedrobaeza No, as said in my commit, the JS rendering part will auto-convert oe_highlight to btn-primary. So no API change here.

I changed the XML for consistency but it is not necessary. We may also want to do the opposite (using oe_highlight everywhere). Both approaches make sense.

pedrobaeza · 2018-02-16T12:28:49Z

Thanks for the explanation. That's better for keeping compatibility, but I will start to change the class in v12 modules with the same consistency spirit 😃

qsm-odoo · 2018-02-16T14:06:25Z

@ged-odoo I split the commit in 3 parts, the last one should be discussed and is not vital. I will also do the same for the oe_link class, so you might want to wait before reviewing.

qsm-odoo · 2018-02-16T14:49:10Z

@aab-odoo the opportunity to assign someone falls on you :)

aab-odoo

Seems good

aab-odoo · 2018-02-19T10:26:04Z

addons/web/static/src/js/views/list/list_renderer.js

-        } else {
-            $button.text(node.attrs.string);
+            var $i = $button.children().detach();
+            $button.empty().append($i);


To do the equivalent as before (icon OR text for list buttons, the icon buttons in lists have no button appearance thanks to the o_icon_button class).

i don't like that :D

Meh, I will parametrize the _renderButtonFromNode function then

aab-odoo · 2018-02-19T10:30:36Z

addons/web/static/src/js/core/dom.js

-        params.class = 'btn btn-' + (options.size || 'sm') + ' ' + (params.class || 'btn-default');
+        var classes = 'btn-default';
+        if (params.class) {
+            classes = params.class.replace(/\boe_highlight\b/g, 'btn-primary')


add comment specifying that this is backward compatibility?

Not sure. Depends if we want to get rid of oe_hightlight in views / use only this class / use a new class / keep using it or btn-primary (as we discussed regarding the last commit of this PR)

i'd be happy if people stop using it... one concept one keyword/classname

Yes, but what keyword? :) btn-primary depends on bootstrap. oe_hightlight... is a crappy name :)

whatever as soon as there is only one

@aab-odoo I changed the PR following your comments, except for this one. Should I merge the PR without the last commit and leave it for some more framework-team-thinking ?

What does @JKE-be think about it? For me it's ok...

@JKE-be What do you think about commit: 5113952 ? The JS code would still replace them automatically with btn-primary but we might want to use only one class in views for consistency (same goes for oe_link/btn-link). Is it worth breaking clients' xpaths ?

aab-odoo · 2018-02-19T10:32:27Z

addons/web/static/src/js/views/form/form_renderer.js

-                .text(node.attrs.string)
-                .appendTo($button);
-        }
+        var $button = this._renderButtonFromNode(node); // FIXME force oe_stat_button ?


maybe... this should probably not even appear in views' arch, and should be automatically added by the webclient

Button rendering was most of the time wrong, incomplete or performed with duplicated code. This led to: - Both primary and default classes were put on some statusbar buttons - Icons were not able to be put for statusbar buttons - Stat buttons and list buttons code were duplicated - Classes and style were added twice - ...

* account, test_main_flows The style of a primary/link button is given by the 'btn-primary'/'btn-link' class. The old 'oe_highlight'/'oe_link' class should still be supported in views but not rendered in the DOM.

The 'oe_link' class is not linked to any style since .... The class is automatically converted to 'btn-link' for <button/> elements but this will not be done on <a/> elements as they are (nearly) unprocessed node during views rendering. Also there is minor difference between a '.btn-link' element and a <a/> element anyway (and maybe there should not be at all).

This is done for pure consistency, the 'oe_highlight' class is still supported. NOTE: we may want to do the opposite or introduce a new class like 'o_primary'.

qsm-odoo · 2018-04-18T15:21:26Z

This was merged with the less2scss convertion except for this commit: a76dc98

@ged-odoo @aab-odoo I let you decide if this must be merged and if we should also stop supporting oe_highlight / oe_link as discussed

qsm-odoo added RD research & development, internal work Framework General frontend/backend framework issues labels Feb 16, 2018

qsm-odoo self-assigned this Feb 16, 2018

qsm-odoo commented Feb 16, 2018

View reviewed changes

qsm-odoo force-pushed the master-highlight-qsm branch from 88d5464 to 1722230 Compare February 16, 2018 14:01

qsm-odoo force-pushed the master-highlight-qsm branch from 1722230 to 3a813dc Compare February 16, 2018 14:31

aab-odoo reviewed Feb 19, 2018

View reviewed changes

qsm-odoo force-pushed the master-highlight-qsm branch 4 times, most recently from ee8833f to 5113952 Compare February 26, 2018 13:41

qsm-odoo added 4 commits February 28, 2018 14:54

[IMP] web, *: do not use 'oe_highlight' and 'oe_link' class in the DOM

975b80f

* account, test_main_flows The style of a primary/link button is given by the 'btn-primary'/'btn-link' class. The old 'oe_highlight'/'oe_link' class should still be supported in views but not rendered in the DOM.

[REF] *: get rid of the 'oe_highlight' class

a76dc98

This is done for pure consistency, the 'oe_highlight' class is still supported. NOTE: we may want to do the opposite or introduce a new class like 'o_primary'.

qsm-odoo force-pushed the master-highlight-qsm branch from 5113952 to a76dc98 Compare February 28, 2018 13:58

qsm-odoo closed this Apr 18, 2018

xmo-odoo deleted the master-highlight-qsm branch November 20, 2019 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of the rendering of views' buttons #23115

Refactoring of the rendering of views' buttons #23115

qsm-odoo commented Feb 16, 2018 •

edited

qsm-odoo Feb 16, 2018

aab-odoo Feb 19, 2018

qsm-odoo commented Feb 16, 2018

pedrobaeza commented Feb 16, 2018

qsm-odoo commented Feb 16, 2018

pedrobaeza commented Feb 16, 2018

qsm-odoo commented Feb 16, 2018

qsm-odoo commented Feb 16, 2018

aab-odoo left a comment

aab-odoo Feb 19, 2018

qsm-odoo Feb 19, 2018

aab-odoo Feb 19, 2018

qsm-odoo Feb 19, 2018 •

edited

aab-odoo Feb 19, 2018

qsm-odoo Feb 19, 2018

aab-odoo Feb 19, 2018

qsm-odoo Feb 19, 2018

aab-odoo Feb 19, 2018

qsm-odoo Feb 26, 2018

aab-odoo Feb 26, 2018

qsm-odoo Feb 26, 2018

aab-odoo Feb 19, 2018

qsm-odoo commented Apr 18, 2018

Refactoring of the rendering of views' buttons #23115

Refactoring of the rendering of views' buttons #23115

Conversation

qsm-odoo commented Feb 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qsm-odoo commented Feb 16, 2018

pedrobaeza commented Feb 16, 2018

qsm-odoo commented Feb 16, 2018

pedrobaeza commented Feb 16, 2018

qsm-odoo commented Feb 16, 2018

qsm-odoo commented Feb 16, 2018

aab-odoo 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

qsm-odoo Feb 19, 2018 • edited

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

qsm-odoo commented Apr 18, 2018

qsm-odoo commented Feb 16, 2018 •

edited

qsm-odoo Feb 19, 2018 •

edited