-
-
Notifications
You must be signed in to change notification settings - Fork 2k
[18.0][MIG] web_remember_tree_column_width #3114
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
[18.0][MIG] web_remember_tree_column_width #3114
Conversation
AaronHForgeFlow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional review 👍 Thank you.
Can't do a proper code review, as my knowledge in js is limited. Added a couple of minor comments.
web_remember_tree_column_width/static/src/js/list_renderer.esm.js
Outdated
Show resolved
Hide resolved
1db02d7 to
f26d5a3
Compare
AaronHForgeFlow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM functional + soft code review 👍
|
This PR has the |
|
Can we get this PR merged? |
|
Can we get this merged ? |
1 similar comment
|
Can we get this merged ? |
|
Can we merge this, please? |
StefanRijnhart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, this looks like a modified copy of https://github.com/odoo/odoo/blob/18.0/addons/web/static/src/views/list/column_width_hook.js. Is it documented anywhere that this is the case? Are the changes annotated properly? Is there a more maintainable way of reusing at least parts of the code in that file?
that is used as a hook, and it is not possible to inherit it |
But items like |
|
In any case, please document the origin of the code and annotate your changes to it. |
… widths Remember the tree columns' widths across sessions, and after filtering, grouping, or reordering.
this fixes scss file not found error after installing the module
…on column Fix an error when retrieving column sizes for lists containing one or more list_button columns. This error occurs when a list_button column is placed between other columns in the list. This type of column is excluded from the querySelector string used to retrieve column headers, which can cause the index to shift and the column size to change when the list is displayed.
|
@JasminSForgeFlow Thanks. You can squash my commit in your migration commit and share credits by mentioning me with |
Sure, I'll update commit. I haven't faced such issue, have you faced an issue with users' last view? |
|
@JasminSForgeFlow Yes, as I indicated I encountered the same error, but uncaught in your version on this PRs runbot. See the screenshot above. I am now catching this condition, but the size cannot be stored in that case. |
But you mentioned that you solved it in your version, didn't you? |
|
@JasminSForgeFlow unfortunately, I did not see how to solve the issue. Like I said,
|
Co-authored-by: Stefan Rijnhart <stefan@opener.amsterdam>
4b7f50c to
b558c28
Compare
Okay, I have added it to know issue, and commit is also updated. |
StefanRijnhart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking my suggestions!
|
This PR has the |
TomAlbrechtQuatra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional & Technical review
Thanks!
|
@OCA/web-maintainers this one should be ready for merge |
etobella
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ocabot merge nobump
|
This PR looks fantastic, let's merge it! |
|
Congratulations, your PR was merged at feca976. Thanks a lot for contributing to OCA. ❤️ |
|
FYI this module broke 4 days ago due to a change in Odoo. Fixed in #3177 |
|
@JasminSForgeFlow This does not work at all in Odoo 18 Enterprise edition. Is it only tested in CE, or is there perhaps something interfering in our database? |
|
It is tested in CE, not in EE, as some of us don't have access to EE. Actually, OCA only tests in CE. If you want, you can try to find the reason and make it compatible. |
|
@Bart-dh Using this in EE, but check the known issue discussed above. For us, it works most of the time. -- edit -- meant to say EE. |
|
I tested it in EE and no issues so far. |
|
@Bart-dh I'm suspecting there may be an issue with list views that contain buttons. Buttons may mess up the mapping of fields to list columns. Are there buttons in the views that don't work for you? |
Ahh yes, indeed there are buttons! There is the default 'View record' button, but also custom buttons. |
Standard Migration
Adapt 18.0 new hook code for change/update width of list
@ForgeFlow