Skip to content

Conversation

@Brieuc-brd
Copy link
Contributor

@Brieuc-brd Brieuc-brd commented Oct 27, 2025

  • html_builder

This commit adds a new carousel snippet that supports displaying multiple slides at once.

task-4094404

Co-authored by: Serhii Rubanskyi [email protected]

Requires:

Capture d’écran 2025-11-27 à 14 21 19

I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo
Copy link
Contributor

robodoo commented Oct 27, 2025

Pull request status dashboard

@Brieuc-brd Brieuc-brd force-pushed the master-imp-website-s_carousel_multiple-brd branch 5 times, most recently from b27a1b4 to ed322c0 Compare October 30, 2025 12:50
@Syozik
Copy link
Contributor

Syozik commented Nov 6, 2025

Heyy :) I will work on the issues you mentioned in the task and resolve the conflicts

@Syozik Syozik force-pushed the master-imp-website-s_carousel_multiple-brd branch 7 times, most recently from 86eda60 to e6ee56e Compare November 12, 2025 12:04
@Syozik
Copy link
Contributor

Syozik commented Nov 12, 2025

Hello @Brieuc-brd, could you please take a look at the functional side of it?

@Brieuc-brd
Copy link
Contributor Author

Hello @Syozik 👋 , thanks for your work!

I spotted two bugs:

For the rest, I’ll leave more in-depth testing to @lebl-odoo 🙂

Thanks !

@Syozik Syozik force-pushed the master-imp-website-s_carousel_multiple-brd branch 2 times, most recently from 3afe706 to 02d75f2 Compare November 17, 2025 08:45
@Syozik
Copy link
Contributor

Syozik commented Nov 17, 2025

Had to force-push it to resolve new conflicts 🙏

@Syozik Syozik force-pushed the master-imp-website-s_carousel_multiple-brd branch 2 times, most recently from ce9aa3d to b39ddcd Compare November 18, 2025 10:06
@Brieuc-brd
Copy link
Contributor Author

Hello @Syozik 👋 ,

Thanks for your work.
I found two technical issues in the latest runbot instance, could you take a look? 🙂

The snippet shouldn’t be editable as plain text content:
https://www.awesomescreenshot.com/video/46528601?key=629e93e50df687489d0c4dbdd07ce88e

The editor overlay only appears when hovering the editor side panel. It seems to happen only the first time you drag and drop a snippet. This issue doesn’t seem to be on master:
https://www.awesomescreenshot.com/video/46527964?key=7a89af3887744b09147c16aba6f0a8fe

Thanks in advance !

cc. @lebl-odoo

@Syozik Syozik force-pushed the master-imp-website-s_carousel_multiple-brd branch from b39ddcd to 855725a Compare November 18, 2025 15:55
@Syozik
Copy link
Contributor

Syozik commented Nov 18, 2025

Hey @Brieuc-brd, Thanks for noticing those 2 issues!
Issue 1 now should be resolved. I made the carousel not editable.

Unfortunately, I couldn't reproduce the second one:
https://www.awesomescreenshot.com/video/46529887?key=7ca6a97c41b022b2e1117a394803fe67
What browser did you use? Was there anything in the dev tools' console?

And also, if you and @lebl-odoo are satisfied with everything else, I guess I can move on to writing test coverage?

@Brieuc-brd
Copy link
Contributor Author

Thanks @Syozik 👍

Unfortunately, I couldn't reproduce the second one:

Weird, I don’t have it anymore either 🤔
Anyway, LGTM 🙂

@lebl-odoo
Copy link

Thx for the ping, LGTM guys @Syozik @Brieuc-brd

@Syozik Syozik force-pushed the master-imp-website-s_carousel_multiple-brd branch 2 times, most recently from baaf378 to 98dbd30 Compare November 26, 2025 09:45
@Syozik
Copy link
Contributor

Syozik commented Nov 26, 2025

Hello @Brieuc-brd, I had a chance to add tests for this pr, so I think you can mark it as ready for review and proceed as usual? Feel free to fixup my commit with yours, change the commit message, or anything else 👍

@Syozik Syozik force-pushed the master-imp-website-s_carousel_multiple-brd branch from 98dbd30 to 02f7819 Compare November 26, 2025 13:54
@Brieuc-brd
Copy link
Contributor Author

Thanks for your work @Syozik 💪
I've squashed our commits and rebased the branch 👍
But since the rebase, it seems there's an error : https://runbot.odoo.com/runbot/build/94352517
Could you check ?

Thanks in advance !

@Syozik
Copy link
Contributor

Syozik commented Nov 27, 2025

@Brieuc-brd
Hmm, it looks like the problem is related to 5291514, it seems the test fails randomly for other builds as well. The author has been notified, so I hope the fix will be deployed shortly 🙏

* html_builder

This commit adds a new carousel snippet that supports displaying
multiple slides at once.

Co-authored by: Serhii Rubanskyi <[email protected]>

task-4094404
@Brieuc-brd Brieuc-brd force-pushed the master-imp-website-s_carousel_multiple-brd branch from 849d64d to ff4e7bb Compare November 27, 2025 13:25
@Brieuc-brd
Copy link
Contributor Author

Alright, thanks @Syozik ! 👍

Hey @odoo/rd-website 👋
Here's a brand new snippet ready for tech. review, could you have a look at it ?
Note : there's a security check : https://runbot.odoo.com/runbot/batch/2254150/build/94364295

Thanks in advance ! 🙏

@Brieuc-brd Brieuc-brd marked this pull request as ready for review November 27, 2025 14:45
@Brieuc-brd Brieuc-brd requested a review from a team November 27, 2025 14:46
@FrancoisGe
Copy link
Contributor

@sobo-odoo Could you check it or assign someone ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants