[18.0][MIG] web_send_message_popup: Migration to 18.0 #3156
Conversation
…to open directly the full featured message popup
* Intitial manifest changes * [FIX] small README tricks
|
@OCA/web-maintainers |
|
@tarteo
In all seriousness I see where you're going with this, but no. |
There was a problem hiding this comment.
After some digging, what you can do is create a callback prop and pass the onClickFullComposer to expose it to the chatter. something like this in the setup function:
if (this.props.someCallback) {
this.props.someCallback({onClickFullComposer: this.onClickFullComposer});
}
|
This PR has the |
|
@tarteo |
|
@dreispt |
This seems a very aggressive language, don't you think? Reading the thread, it seems there are suggestions to improve the code. Have they been considered? |
|
@pedrobaeza @tarteo But at the end of the day, if merging this means doing that, well... I don't have a choice I guess. |
|
Glad to hear that it was just a joke. I think most of us thought otherwise. The written language + the lack of context makes it very easily misunderstood, so please take it into account for other times in the future. Thanks for the extra explanation about why not changing it. I lack a bit of knowledge in JS, but seems reasonable. What you have to do in that changes, is to put a comment over the code stating that reasons for doing it that way, as they seem not obvious - don't do that with every obvious thing - so other developers looking at the code (or you future you) remember why it was done that way and why a refactoring "reducing" code is not convenient, so you avoid the temptation next time of this being proposed to be changed again. Can you please add such comment and we proceed with the merge? |
b9914a9 to
427bc1e
Compare
|
I already suspected it was a cultural difference or language barrier. What I'm suggesting is not unorthodox, it's a common way to do it: https://github.com/odoo/owl/blob/master/doc/reference/props.md#binding-function-props It's even proposed if you use deprecated t-ref attribute: |
|
But I'm fine merging it with the clarifying comments |
|
@tarteo And you're right about callback methods, they're fine! It's just a bunch of forth and back, it can get confusing and it doesn't sound right... @pedrobaeza |
|
/ocabot merge nobump |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Thanks everybody for the constructive technical debate! |
|
/ocabot migration web_send_message_popup |
|
Congratulations, your PR was merged at bcd9ad9. Thanks a lot for contributing to OCA. ❤️ |
Module migration from
16.0straight to18.0, cba with17.0.Made sure
pre-commitdoesn't give warnings on import sorting (unlike some modules...).Massive .js changes, previous version (
16.0) .js code was absolutely incompatible.