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

[IMP] website: Mega Menu #31852

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@kea14
Copy link
Contributor

kea14 commented Mar 14, 2019

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@C3POdoo C3POdoo added the RD label Mar 14, 2019

@kea14 kea14 force-pushed the odoo-dev:master-website-mega-menu-aul branch from 675c9f0 to 3fb810a Mar 19, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 19, 2019

@kea14 kea14 force-pushed the odoo-dev:master-website-mega-menu-aul branch from 3fb810a to cb57833 Mar 20, 2019

@robodoo robodoo removed the CI 🤖 label Mar 20, 2019

@kea14 kea14 requested a review from qsm-odoo Mar 20, 2019

@qsm-odoo
Copy link
Contributor

qsm-odoo left a comment

Just a quick check you are going in the right direction, I did not look at everything. It seems ok, be careful to not scatter too much. Focus on one point then validate your work with me / jke, then continue, etc.

@@ -26,6 +26,7 @@ def _default_sequence(self):
child_id = fields.One2many('website.menu', 'parent_id', string='Child Menus')
parent_path = fields.Char(index=True)
is_visible = fields.Boolean(compute='_compute_visible', string='Is Visible')
mega_menu_content = fields.Html(string='Mega Menu Content', translate=True)

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 20, 2019

Contributor
  • The string you specific is the default one (guessed from your variable name), so no need to define it.
  • Check the other definitions of Html fields used in the website, the value for translate should not be True ;)
@@ -114,6 +115,7 @@ def make_tree(node):
parent_id=node.parent_id.id,
children=[],
is_homepage=is_homepage,
is_mega_menu=isinstance(node.mega_menu_content, str),

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 20, 2019

Contributor

I think bool(node.mega_menu_content) is cleaner and easier to read

content = False
else:
content = """
<div class="dropdown-menu" aria-labelledby="dropdownMenuButton">

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 20, 2019

Contributor

No aria-... stuff in this case

@@ -1154,3 +1154,45 @@ table.table_desc tr td {
.modal-footer > .float-left {
margin-right: auto;
}

.dropdown-menu {

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 20, 2019

Contributor

I don't think any css is needed in this task for now, standard bootstrap should be enough I think

@robodoo robodoo added the CI 🤖 label Mar 20, 2019

@kea14 kea14 force-pushed the odoo-dev:master-website-mega-menu-aul branch from cb57833 to 829771c Mar 21, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 21, 2019

@kea14 kea14 force-pushed the odoo-dev:master-website-mega-menu-aul branch from 829771c to 6be6cda Mar 22, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 22, 2019

m

@kea14 kea14 force-pushed the odoo-dev:master-website-mega-menu-aul branch from 6be6cda to 5387645 Mar 26, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 26, 2019

@qsm-odoo
Copy link
Contributor

qsm-odoo left a comment

New review to see how the task is going on :)
Also two remarks:

  • Cannot toggle mega menu on a newly created menu item
  • Do not forget that the main goal is to have full width menus with columns, images, etc, creating simple dropdown menus was already possible ;)
*
* @private
*/
_toggleMegaMenuSnippets: function (hide) {

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

Do not forget to add @param in the JSDoc. When toggling with the true value, it should show not hide 😉

@@ -26,6 +26,7 @@ def _default_sequence(self):
child_id = fields.One2many('website.menu', 'parent_id', string='Child Menus')
parent_path = fields.Char(index=True)
is_visible = fields.Boolean(compute='_compute_visible', string='Is Visible')
mega_menu_content = fields.Html('Mega Menu Content')

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

I was not saying that the field should not be translated :)

<a class="dropdown-item" href="#">Another action</a>
<a class="dropdown-item" href="#">Something else here</a>
</div>
"""

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

Put this in a XML template (with an ID like default_...) and render its content here.


@api.model
def toggle_mega_menu(self, data):
menu_item = self.browse(data['data']['id'])

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

It is a model method which first browse a record and do something on it... this should probably be a @api.multi method to avoid having to browse :)

@@ -215,6 +215,8 @@ var EditorMenu = Widget.extend({
* @private
*/
_onSaveClick: function () {
// Close all dropdowns menu to avoid saving opened + edited dropdown
$('.dropdown-menu').dropdown('hide');

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

There are systems to do something when the editor is saved, you cannot rely that it is always done through the _onSaveClick handler. There probably are triggered events when the editor is saved and/or something related to the cleanForSave methods.

<i t-if="submenu.is_homepage" class="fa fa-home ml-3" role="img" aria-label="Home" title="Home"/>
</span>
<span class="input-group-append">
<button type="button" t-attf-class="btn btn-info js_convert_mega_menu fa fa-sitemap" t-attf-aria-label="#{submenu.is_mega_menu ? 'Remove Mega Menu' : 'Convert into Mega Menu'}" t-attf-title="#{submenu.is_mega_menu ? 'Remove Mega Menu' : 'Convert into Mega Menu'}"/>

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

Twice the same condition and making them that way will not make the strings translatable. Before the button, set a variable this way:

<t t-set="my_var">
    <t t-if="submenu.is_mega_menu">...</t>
    <t t-else="">...</t>
</t>

and use a trimmed version of the variable as title and aria-label.

@@ -818,6 +818,59 @@
</div>
</template>

<template id="s_link_menu" name="Link">
<div class="o_link_menu">

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

For snippets, start the classnames with s_ instead of o_

@@ -1354,5 +1424,13 @@
data-selector=":not(p).oe_structure > *, :not(p)[data-oe-type=html] > *, .row > div">
<a tabindex="-1" href="#" class="dropdown-item" data-no-preview="true" data-open-anchor-dialog=""><i class="fa fa-anchor"/>Anchor Name</a>
</div>

<div id="mega_menu_snippets"
data-selector=".o_link_menu, .o_sublink_menu, .o_dropdown_menu, .o_title_menu, .o_subtitle_menu, .o_image_menu, .o_column_menu, .o_divider_menu"

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

Add a class like s_mega_menu_item on all snippets and only mention this class here instead of all the snippet ones 😉

@@ -132,7 +132,8 @@
<!-- Layout -->
<template id="submenu" name="Submenu">
<t t-set="has_visible_submenu" t-value="submenu.child_id.filtered(lambda menu: menu.is_visible)"/>
<li t-if="submenu.is_visible and not has_visible_submenu" t-attf-class="#{item_class or ''}">
<t t-set="has_mega_menu_content" t-value="submenu.mega_menu_content"/>

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

Not really useful to put this in a separated variable, using submenu.mega_menu_content directly seems clear enough

@@ -157,6 +158,34 @@
</t>
</ul>
</li>
<li t-if="has_mega_menu_content" t-attf-class="nav-item dropdown">
<a href="#" class="nav-link dropdown-toggle" data-toggle="dropdown">
<span t-field="submenu.name"/><b class="caret"></b>

This comment has been minimized.

@qsm-odoo

qsm-odoo Mar 26, 2019

Contributor

I don't think the caret class exist in BS4 ?

@kea14

This comment has been minimized.

Copy link
Contributor Author

kea14 commented Mar 26, 2019

Thanks :)

Do not forget that the main goal is to have full width menus with columns, images, etc, creating simple dropdown menus was already possible ;)

Agree 100%, so maybe should I not use dropdowns but directly something like a full-width div ?

@qsm-odoo

This comment has been minimized.

Copy link
Contributor

qsm-odoo commented Mar 26, 2019

Yes, probably, and the user should probably be allowed to resize it or use snippet options to choose different sizes, ... to test :)

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.