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

[16.0][ADD] web_widget_product_label_section_and_note: New module to unify … #3009

Merged

Conversation

carlos-lopez-tecnativa
Copy link
Contributor

@carlos-lopez-tecnativa carlos-lopez-tecnativa commented Dec 3, 2024

…the product and name into a single column.
Backport of functionality present in V18.

TT51520
@Tecnativa @pedrobaeza @chienandalu could you please review this

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Functional review on runboat. Looks perfectly great.

thanks for your work.

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Note : it looks like there is a change in the PDF.
Native : display name on PDF

Now :

  • if product_id and NOT name : display product_id
  • if product_id and name : display both

It is maybe a regression if user dont want to display the product_id field.

@carlos-lopez-tecnativa
Copy link
Contributor Author

Note : it looks like there is a change in the PDF. Native : display name on PDF

Now :

  • if product_id and NOT name : display product_id
  • if product_id and name : display both

It is maybe a regression if user dont want to display the product_id field.

Sorry, but I don't understand your comment. This code is not related to printing PDFs; it's more related to when the user prints the screen using the native browser options (Ctrl + P). Could you please clarify what you're saying? Thanks a lot.

https://github.com/OCA/web/pull/3009/files#diff-b1fa0eeac3583410b62e991f1091d3164d7970b2bf67878a5e3663c8cfc88314R156-R175

@legalsylvain
Copy link
Contributor

legalsylvain commented Dec 5, 2024

Hard to explain :

Without the module, in Odoo :

image

Related PDF :

image

See, the third line, when we overwrite the name ("label") field, we can remove the product name product code, and the line displayed only contains "With replacement" text. User can totally customize the text of the invoice.

Now, install the module, and add a new "with replacement line".

With the module, in Odoo :

image

Related PDF :

image

As you can see, the fourth line, contains the name and the code of the product, and it is not possible to hide it. Full customization is not possible anymore.

please ping me if I'm not clear.

thanks !

@carlos-lopez-tecnativa
Copy link
Contributor Author

carlos-lopez-tecnativa commented Dec 5, 2024

@legalsylvain Thanks for explaining it to me. I tested it in V18, and the result is the same. So, as this is a backport, the behavior is as expected. I think it can be reported to Odoo instead. What do you think? (I understand that in V16, it is not the expected behavior.) cc @pedrobaeza

@legalsylvain
Copy link
Contributor

legalsylvain commented Dec 5, 2024

Hi @carlos-lopez-tecnativa. First thanks for the comparison with V18 version. I checked also on a fresh V18 fresh database.
My point of view :

  • in your backport, in V16, just add a warning or some text somewhere in the description, to explain that change, because the installation of the module "web_widget_product_label_section_and_note" will change the native feature of odoo account V16 module. IMO, the user that is installing the module should know that.
  • in V18, it is native. In fact it's a design change. Not sure it's a bug. Yes, now, product name and code are always displayed. Maybe it's the intention of odoo team. I'm not working on odoo 18 version. If you think it's a bug (or if your customer is not agree with that change, feel free to open an issue).

On another note. thanks a lot for this backport ! It's a very great improvment of V18 version, and it's great to have it available on V16 version. The invoice form view is very lighter.

@carlos-lopez-tecnativa carlos-lopez-tecnativa force-pushed the 16.0-web_widget_product_label_section_and_note branch from a5b5cdb to ff43074 Compare December 5, 2024 20:18
@carlos-lopez-tecnativa
Copy link
Contributor Author

  • in your backport, in V16, just add a warning or some text somewhere in the description, to explain that change, because the installation of the module "web_widget_product_label_section_and_note" will change the native feature of odoo account V16 module. IMO, the user that is installing the module should know that.

@legalsylvain I added a note in the ROADMAP. Please let me know if it’s okay for you.

@pedrobaeza
Copy link
Member

Ouch, indeed that's a big limitation of 18.0 (and by extension, of this widget in 16.0). I have opened an issue in Odoo to gather their opinion on this:

odoo/odoo#189870

Meanwhile, what do you think about the design solution I have proposed in it?

@carlos-lopez-tecnativa I think it can be doable adding the interface and touching the method that computes the displayed name, isn't it? In that case, we may include it in this widget for 16/17, and on 18, if Odoo doesn't perform any change, extract just this part in a module to be plugged to the core widget.

@carlos-lopez-tecnativa carlos-lopez-tecnativa force-pushed the 16.0-web_widget_product_label_section_and_note branch from ff43074 to 7aa7851 Compare December 10, 2024 19:57
@carlos-lopez-tecnativa
Copy link
Contributor Author

@legalsylvain @pedrobaeza
I added the new icon to the widget according to Pedro's suggestion to add or remove the product name from the description. Now, it’s possible to remove all product names and keep only the name as before. Please update your review. Thanks in advance.
image

@pedrobaeza pedrobaeza added this to the 16.0 milestone Dec 11, 2024
"website": "https://github.com/OCA/web",
"depends": [
"web",
"account",
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't depend on account. This have to be just the widget.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This dependency is necessary because the base component is created in the account module, please see:
https://github.com/odoo/odoo/tree/16.0/addons/account/static/src/components/section_and_note_fields_backend

Copy link
Member

Choose a reason for hiding this comment

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

OK, understood.

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 border is ugly and anti-ergonomic:

image

I think it's not done this way in 18.

Other thing is that I have put the product name as visible with the eye icon, but it's still not printed.

image

image

@carlos-lopez-tecnativa
Copy link
Contributor Author

This border is ugly and anti-ergonomic:

This border is added by the web_theme_classic module
image

Without this module, the border is not displayed. It is necessary to remove the border in this specific component by overriding the style from web_theme_classic. Please confirm this change; I think it is unnecessary.

image

@pedrobaeza
Copy link
Member

OK about the border. That awful anti-ergonomic theme... Nothing is needed to be done for this thing.

Only the other problem about the product name not being showed remains.

@carlos-lopez-tecnativa
Copy link
Contributor Author

Other thing is that I have put the product name as visible with the eye icon, but it's still not printed.

image

image

I tested it, and it’s working fine. The initial icon is fa-eye-slash to indicate that when the user clicks this button, the product name is removed, and the icon updates to fa-eye (your current state). When the icon is fa-eye, if the user clicks again, the product name is added back. However, you didn’t click a second time. This works as a toggle. Please try again.
image

image

@pedrobaeza
Copy link
Member

pedrobaeza commented Dec 11, 2024

Then it shouldn't be the contrary: if I see the icon fa-eye-slash, I expect to not have the description, not to have it. The icon should reflect the current status.

@carlos-lopez-tecnativa
Copy link
Contributor Author

Then it shouldn't be the contrary: if I see the icon fa-eye-slash, I expect to not have the description, not to have it. The icon should reflect the current status.

Ok, I understand your point. My logic was to reflect the action the user will perform, not the current state.

@carlos-lopez-tecnativa carlos-lopez-tecnativa force-pushed the 16.0-web_widget_product_label_section_and_note branch from 7aa7851 to b21e41d Compare December 11, 2024 13:26
@carlos-lopez-tecnativa
Copy link
Contributor Author

Then it shouldn't be the contrary: if I see the icon fa-eye-slash, I expect to not have the description, not to have it. The icon should reflect the current status.

@pedrobaeza I updated the icon to reflect the current state. Please review again. Thanks in advance.

@carlos-lopez-tecnativa carlos-lopez-tecnativa force-pushed the 16.0-web_widget_product_label_section_and_note branch from b21e41d to 2c56620 Compare December 11, 2024 13:52
@carlos-lopez-tecnativa carlos-lopez-tecnativa force-pushed the 16.0-web_widget_product_label_section_and_note branch from 2c56620 to 39b5f39 Compare December 11, 2024 13:53
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.

Thank you.

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-3009-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 8f435b9 into OCA:16.0 Dec 11, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at a93a516. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza pedrobaeza deleted the 16.0-web_widget_product_label_section_and_note branch December 11, 2024 14:57
@legalsylvain
Copy link
Contributor

OK about the border. That awful anti-ergonomic theme... Nothing is needed to be done for this thing.

I hadn't read that this theme could upset some people.
"Ugly", I can understand, as it is a matter of taste, but "anti-ergonomics", I don't get it. Feel free to elaborate.

Anyway, that said, the fact that this module is installed by default on the OCA runboat of one of the most popular repo (OCA/web) is irrelevant, as it disturbs the understanding of PR reviewers. (as it did for you just now).

A solution might be to move this module to a new repo, OCA/web-themes? That way, it wouldn't disrupt the existing repo.

After all, there is such a dedicated repo for odoo (https://github.com/odoo/design-themes)

Let me know !

@pedrobaeza
Copy link
Member

pedrobaeza commented Dec 11, 2024

Yes, it can be a solution. I say anti-ergonomics, because your sight suffers when you have more things in the screen, although being just grey lines. That's why Odoo, when removing the edit view, removed these lines. Having them and using Odoo constantly with this modification may lead to tired eyesight and similar. Another thing that leads to this is to write all in upper (like contact names and/or product names), but this is something of content, not of the base system, and it's a constant fight with users.

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.

4 participants