8000 [16.0][IMP] web_chatter_position: implement feature on controller and add switch button by trisdoan · Pull Request #3040 · OCA/web · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[16.0][IMP] web_chatter_position: implement feature on controller and add switch button #3040

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: 16.0
Choose a base branch
from
Open

[16.0][IMP] web_chatter_position: implement feature on controller and add switch button #3040

wants to merge 3 commits into from

Conversation

trisdoan
Copy link
Contributor

Context

  • The Switch Button was dropped during migration to 16.0, which I assumed it was due to the approach taken at that time.
    • In Odoo 15, the position was patched in Controller and Renderer and adjusted by DOM
    • In Odoo 16, it was chosen to do it via Compiler, which the component (Form) needs to be destroyed to re-trigger the Compiler.
    • There was a comment about it but probably missed.

This changes

  • Revert to the approach implemented on Odoo 15.0: by taking advantage of OWL cycles
  • Re-introduce the switch button

Result

  1. No Attachment present
web_chatter_result.webm
  1. Attachment present
    web_chatter_result-(2).webm

@trisdoan
Copy link
Contributor Author

Hello @ivantodorovich, would you mind taking a look at this

@trisdoan trisdoan changed the title [IMP] web_chatter_position: implement feature on controller and add switch button [16.0][IMP] web_chatter_position: implement feature on controller and add switch button Dec 26, 2024
@trisdoan trisdoan force-pushed the 16.0-imp-web_chatter_position-add-switch-button branch from 6943a08 to 21e030a Compare December 27, 2024 07:46
@trisdoan trisdoan marked this pull request as draft December 27, 2024 09:52
@trisdoan
Copy link
Contributor Author
trisdoan commented Dec 27, 2024 •
edited
Loading

DRAFT: event listeners are lost when chatter is at the bottom

@trisdoan trisdoan force-pushed the 16.0-imp-web_chatter_position-add-switch-button branch from 21e030a to 4ec2e8b Compare December 30, 2024 03:55
@trisdoan trisdoan marked this pull request as ready for review December 30, 2024 03:55
@fanha99
Copy link
Contributor
fanha99 commented Dec 30, 2024

nice

@trisdoan trisdoan mentioned this pull request Dec 30, 2024
Open
@trisdoan trisdoan force-pushed the 16.0-imp-web_chatter_position-add-switch-button branch from 4ec2e8b to c5188d4 Compare January 6, 2025 07:22
@ivantodorovich
Copy link
Contributor

Hello!

Thanks for your contribution!

I remember this was an intentional compromise to keep it simple. Hopefully you find a way to achieve it, though.

Did a few functional tests, and unfortunately it doesn't seem to work properly. The side panel show on smaller screens.

Screen.Recording.2025-01-07.at.9.39.24.AM.mov

@trisdoan trisdoan force-pushed the 16.0-imp-web_chatter_position-add-switch-button branch 3 times, most recently from 21714b6 to 2bbf73f Compare January 8, 2025 05:17
@trisdoan
Copy link
Contributor Author
trisdoan commented Jan 8, 2025 •
edited
Loading

Hello @ivantodorovich, I updated the code, how does it look to you?

Why it happened

  • When option is sided and the screen is small (particularly smaller than XXL), the chatter is hidden.

Solution

  • Add a check (if the screen is big enough) before patching the position of the chatter. If it's too small, let the chatter handled by standard
  • Add class: d-none d-xxl-inline-block to hide the switch dynamically whenever the screen is too small

@diggy128
Copy link

Is there a plan to merge this?

@trisdoan
< 8000 /span> Copy link
Contributor Author

Is there a plan to merge this?

Hello, an approval will move up the process faster 😉

@diggy128
Copy link

It seems runboat is stuck.
Can someone rerun the build?

@trisdoan trisdoan force-pushed the 16.0-imp-web_chatter_position-add-switch-button branch from 2bbf73f to 33c4c83 Compare March 31, 2025 01:56
@diggy128
Copy link
diggy128 commented May 3, 2025

Functional review, LGTM!

@trisdoan
Copy link
Contributor Author
trisdoan commented May 6, 2025

Hello @ivantodorovich, how does it look to you?

@diggy128
Copy link

Functional review, LGTM!

It seems there are issues...
I randomly get:
UncaughtClientError > TypeError Uncaught Javascript Error > Cannot read properties of null (reading 'querySelector') TypeError: Cannot read properties of null (reading 'querySelector') at AccountMoveController._moveChatter (https://www.xxx.gr/web/assets/139442-2307463/web.assets_backend.min.js:25546:290) at https://www.xxx.gr/web/assets/139442-2307463/web.assets_backend.min.js:25544:768

It seems it has to do with resizing the window somehow, because it happened once on this occasion, but so far I haven't pinpointed what raises the error, and could not raise it in debug assets mode.

@trisdoan trisdoan force-pushed the 16.0-imp-web_chatter_position-add-switch-button branch from 33c4c83 to 065e15f Compare May 23, 2025 09:40
@trisdoan
Copy link
Contributor Author

Hello @diggy128, thanks for testing. I just pushed a fixup

  1. My assumption is that this.rootRef.el is somehow lost when referring in _moveChatter, although it was checked in moveChatter. Could you help to test it again?
  2. I replaced hasAttachmentViewerinArch with func hasAttachmentViewer() as it did not change position, when module account is installed. Did you get that bug too?

@diggy128
Copy link
diggy128 commented May 23, 2025 •
edited
Loading

OK, I've merged the patch in my dev machine.
Just give me a few days to check if it comes up again.

As for point 2 I can't tell as I didn't keep the error logs. I'll check though if it happens and will let you know.

@diggy128
Copy link
diggy128 commented Jun 4, 2025

Still getting an error (less often, still not repeatable).
One occasion I've noticed is when detaching a tab to a new window.

The error is:
UncaughtClientError > TypeError Uncaught Javascript Error > this.rootRef.el is null TypeError: this.rootRef.el is null setup/<@https://www.xxx.gr/web/assets/140234-8a2f3d1/web.assets_backend.min.js:25544:798

@trisdoan
Copy link
Contributor Author

Hello @diggy128, so much thanks to your testing!
I just pushed a patch to use Odoo built-in function to handle the eventlistner and some defensive checks.

Could you please try again?

@diggy128
Copy link

Hi @trisdoan, happy to help.
I've patched my dev installation and I'll get back to you in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trisdoan @fanha99 @ivantodorovich @diggy128
0