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

[ADD][web_widget_domain_v11] Domain widget backport #672

Merged
merged 2 commits into from
Jul 25, 2017

Conversation

yajo
Copy link
Member

@yajo yajo commented Jul 12, 2017

  • Most code directly copied from upstream Odoo, in the commit before the JS refactor that took place in master.
  • Some little glue code to make it replace v10's widget.

Below, screenshot of mass mailing addon with this addon installed along with it. Read mode:

captura de pantalla de 2017-07-12 12-39-42

Edit mode:

captura de pantalla de 2017-07-12 12-39-53

@Tecnativa

@yajo yajo self-assigned this Jul 12, 2017
@yajo yajo added this to the 10.0 milestone Jul 12, 2017
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module deserves a better icon, like this one:

icon

There's also a problem on advanced search, because the view is very short, so is there a way to expand the size of the popup when selecting "is in selection"? If not, expand it for all the cases at least:

seleccion_009

Please change also the web_advanced_search_x2x README according the changes, and bump its version to 10.0.2.0.0, as it's a big change.


To use this module, you need to:

#. Install any addon that makes use of the domain widget (i.e.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, for being auto-testable, we are going to make the following: change the widget of the domain field of ir.filters. This way, we get that functionality which is very great in this case, and we can test it directly

======================

* This addon replaces the built-in ``char_domain`` widget, so it can break
compatibility with other addons that use it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add also / extend it

@@ -0,0 +1,47 @@
odoo.define("web.DomainSelectorDialog", function (require) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these files needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are thousands of JS lines needed. I just copy-pasted all those involved and made sure there was no module name conflict, but honestly I'd prefer not to have to look deeply into each one of them since the life span of this module is going to be so short (just 1 version).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I was talking about the suspicious name copied-js, that it seems to work like a copied-pasted version for references, but not the final JS.

@pedrobaeza
Copy link
Member

One last thing: translations!

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so great to have this! 😃

Besides @pedrobaeza comments, I found strange not to be able to use the classic domain filtering like here:

image

Is it on purpose or am I missing something?

@ged-odoo
Copy link

good job guys :)

@yajo
Copy link
Member Author

yajo commented Jul 20, 2017

It should be ready now.

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eyeball code review: ok.
functional test: ok.
The only glitch I find is that when you use the widget in a popup (from a related field for instance) the dropdown is cut where the popup ends. But I think this is an issue on the original implementation. Thanks for the port!

@yajo
Copy link
Member Author

yajo commented Jul 20, 2017

@simahawk If you can guide me to get to that, I can look at it. 😉 What steps did you follow? Thanks

@simahawk
Copy link
Contributor

@yajo this is it:

  1. you have a model (A) w/ a field that relates to another model (B)
  2. you click to edit B and you get its popup form as usual
  3. B has a domain field that uses the widget and here's how it looks like in the popup

domain_widget_popup_issue

so that is fight of scrolling because you have to scroll both on the popup and inside the dropdown of the widget. If I open B form straight (not in a popup) it works smoothly.

@yajo
Copy link
Member Author

yajo commented Jul 21, 2017

@simahawk OK I checked it, but it seems like it's good as it is. On one hand I wouldn't know how to properly fix it right now (it's got the proper z-index already AFAICS), but OTOH how would you scroll the main window if there were a modal inside? Just scroll the modal and you can use it as usual, don't you?

@simahawk
Copy link
Contributor

@yajo I know. I was not complaining I was just pointing out the issue 😄

Nevertheless if the modal is not that high scrolling is really a pain. The real issue comes not from z-index but from the modal hiding overflow. A simple (?) fix could be: when the dropdown is shown it sets

.modal .modal-dialog .modal-content .modal-body {overflow: visible;}

on the closest modal? Just tried on the fly and it works. Not sure if it has side effects.

@yajo
Copy link
Member Author

yajo commented Jul 21, 2017

I tested live with your example code, and the problem is that now the modal does not fit into the screen if it is too big.

You can try on current runbot here pressing edit and then on bank_ids for example.

It turns out somebody already hit this problem, and the new problem from your solution, but still I tested that and it does not seem to work in Odoo, so not sure what else to do. It seems this comment nails it, but I don't want to do that, honestly. 😅

@simahawk
Copy link
Contributor

np, we can live with it ATM ;)

@pedrobaeza
Copy link
Member

One last thing, @yajo: can we have inside the widget domain also the possibility to select the many2one value when the operator is "equal" or "not equal" the same as we have it on the "simple search"?

@yajo
Copy link
Member Author

yajo commented Jul 25, 2017

I think that would be a little bit tricky to achieve. Besides, you already have the normal "[not ]equal" filter for that, and I'd prefer not to diverge too much from upstream's domain widget to be able to completely get rid of web_widget_domain_v11 in v11.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it's something that we are losing from previous implementation, but we win a lot of other things, so let's merge this.

yajo added 2 commits July 25, 2017 09:39
- Most code directly copied from upstream Odoo, in the commit before the JS refactor that took place in master.
- Some little glue code to make it replace v10's widget.
@pedrobaeza pedrobaeza merged commit 21f50a6 into OCA:10.0 Jul 25, 2017
@pedrobaeza pedrobaeza deleted the 10.0-web_widget_domain_v11 branch July 25, 2017 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants