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

Feature 22921 mig colorize field in tree views jpr #4

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

dz0
Copy link

@dz0 dz0 commented Mar 12, 2024

Copy link

codecov bot commented Mar 12, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Implementation now is based on updating `style` attribute of `cell/td` instead of directly manipulating element's CSS value.

Cleanup docs about no more implemented `colors` parameter for `tree`
@dz0 dz0 force-pushed the feature-22921-mig-Colorize_field_in_tree_views-jpr branch from 4b4792b to a09db11 Compare March 12, 2024 13:09
Copy link

@maciej-wichowski maciej-wichowski left a comment

Choose a reason for hiding this comment

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

FYI, when pushing not migrated module to 17.0-versada, you should set "installable": False in __manifest__.py. Branch 17.0-versada is for our internal use, so if someone would add it into his project, Odoo would reject modules on init due to module version mismatch.

@@ -529,13 +507,15 @@ <h1>Contributors</h1>
<li>Guewen Baconnier &lt;<a class="reference external" href="mailto:guewen.baconnier&#64;camptocamp.com">guewen.baconnier&#64;camptocamp.com</a>&gt;</li>
<li>Phuc Tran Thanh &lt;<a class="reference external" href="mailto:phuc&#64;trobz.com">phuc&#64;trobz.com</a>&gt;</li>
<li>Sylvain LE GAL &lt;<a class="reference external" href="https://twitter.com/legalsylvain">https://twitter.com/legalsylvain</a>&gt;</li>
<li>Jurgis Pralgauskis &lt;<a class="reference external" href="mailto:jurgis.pralgauskis&#64;gmail.com">jurgis.pralgauskis&#64;gmail.com</a>&gt;</li>

Choose a reason for hiding this comment

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

Usually we provide our @versada.eu email

Choose a reason for hiding this comment

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

Don't modify index.html. It is generated automatically.

Copy link
Author

@dz0 dz0 Mar 13, 2024

Choose a reason for hiding this comment

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

yes, it is taken from CONTRIBUTORS.md, but I got Maciejs idea :)

Comment on lines -84 to -103
With this example, column which renders 'name' field will have its text colored in white on a customer records.

- In the tree view declaration, use
``options='"color_field": "my_color"'`` attribute in the ``tree``
tag:

::

...
<field name="arch" type="xml">
<tree string="View name" colors="color_field: my_color" >
...
<field name="my_color" invisible="1"/>
...
</tree>
</field>
...

- You can also use ``colors="bg_color_field: my_color"`` to defined the
field name that will be used for the background color of the line.

Choose a reason for hiding this comment

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

Why this example is removed?

Copy link
Author

@dz0 dz0 Mar 12, 2024

Choose a reason for hiding this comment

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

I thought it refers to the deprecated functionality... (Oct 14, 2022)
https://github.com/OCA/web/blob/15.0/web_tree_dynamic_colored_field/readme/ROADMAP.rst
OCA@4b1f3fe

Now I noticed the removal was kind of "reverted" 3 days later (Oct 17, 2022)
OCA@9994bc6

So: 1) it is confusing, 2) we didn't use it in Siramis, and 3) I didn't want to spend time on this...
May be you have ideas how to handle this?

PS: whole JS code history: https://github.com/OCA/web/commits/15.0/web_tree_dynamic_colored_field/static/src/js/web_tree_dynamic_colored_field.js

Choose a reason for hiding this comment

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

Not sure why you need to touch this..:)

Copy link
Author

Choose a reason for hiding this comment

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

what do you mean by "this"?

if the docs part -- because I want docs to represent reality...

Choose a reason for hiding this comment

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

Well usually when we migrate OCA modules, we try to cover whole functionality, not only the one we need. Your PR might be rejected (not so bad, since we have our own branch).
So let's discuss. @oerp-odoo @DarkoNikolovski should we invest more time to migrate whole functionality?

Choose a reason for hiding this comment

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

If we can spare some time to properly migrate a module, then we should do it. Maybe @DarkoNikolovski can comment more on that:)

Choose a reason for hiding this comment

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

@dz0 Is it possible to migrate the missing part? If yes what would be the estimated time for that?

Copy link
Author

Choose a reason for hiding this comment

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

seems the functionality can't work, because of reason in ROADMAP.

tried on 17, the schema does not allow colors attr for tree
paveikslas

Choose a reason for hiding this comment

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

If not possible then let's continue as it is.
Wondering how it is working in v.15 or is it working at all, because I can't see colors there as well

@dz0
Copy link
Author

dz0 commented Mar 12, 2024

FYI, when pushing not migrated module to 17.0-versada, you should set "installable": False in __manifest__.py. Branch 17.0-versada is for our internal use, so if someone would add it into his project, Odoo would reject modules on init due to module version mismatch.

Ok, I will know,
but probably I can skip this in our case -- as noone ese uses 17.0-versada web yet, and before we merge migration...

@dz0
Copy link
Author

dz0 commented Mar 12, 2024

By the way,
is it ok, that I squashed quite some (auxilary) commits in history till 15.0 ?
(as it was recommended)

image

@DarkoNikolovski DarkoNikolovski requested review from oerp-odoo and removed request for DarkoNikolovski March 12, 2024 18:05
@oerp-odoo
Copy link

FYI, when pushing not migrated module to 17.0-versada, you should set "installable": False in __manifest__.py. Branch 17.0-versada is for our internal use, so if someone would add it into his project, Odoo would reject modules on init due to module version mismatch.

Why do we need to merge not migrated module? You could commit on your main branch yo reference your web branch you are working on, no need to merge anything to 17.0-versada before you migrate. Once it is migrated, then you can merge it here and switch in our monorepo as well.

@@ -1,3 +1,4 @@
The development of this module has been financially supported by:

- Camptocamp
- Versada.eu

Choose a reason for hiding this comment

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

Suggested change
- Versada.eu
- Versada

</ul>
</div>
<div class="section" id="other-credits">
<h1>Other credits</h1>
<p>The development of this module has been financially supported by:</p>
<ul class="simple">
<li>Camptocamp</li>
<li>Versada.eu</li>

Choose a reason for hiding this comment

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

Don't modify index.html for OCA modules, it is generated from rst files

Suggested change
<li>Versada.eu</li>

@dz0
Copy link
Author

dz0 commented Mar 13, 2024

FYI, when pushing not migrated module to 17.0-versada, you should set "installable": False in __manifest__.py. Branch 17.0-versada is for our internal use, so if someone would add it into his project, Odoo would reject modules on init due to module version mismatch.

Why do we need to merge not migrated module? You could commit on your main branch yo reference your web branch you are working on, no need to merge anything to 17.0-versada before you migrate. Once it is migrated, then you can merge it here and switch in our monorepo as well.

I thought then PR will be harder to track (and I didn't change any code "directly" in that "historical" part)....
But yews, we could select just commits with our changes for mine-changes review...

@dz0 dz0 requested a review from oerp-odoo March 13, 2024 08:52
@@ -515,7 +515,7 @@ <h1>Other credits</h1>
<p>The development of this module has been financially supported by:</p>
<ul class="simple">
<li>Camptocamp</li>
<li>Versada.eu</li>
<li>Versada</li>

Choose a reason for hiding this comment

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

As I said, you must not edit index.html for OCA modules, this is generated automatically by OCA tools. You have to modify only rst files which index is generated from

Copy link
Author

Choose a reason for hiding this comment

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

hmm, I didn't -- pre-commit did 🤷

@dz0 dz0 merged commit 384ba04 into 17.0-versada Mar 14, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants