add carousel option #4250
Merged
add carousel option #4250
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg, it needs to be retargeted before it can be merged. |
Jinjiu96
force-pushed
the
master-mysterious-egg-jili-1
branch
6 times, most recently
from
54c3bee to
7b7f49a
Compare
robinlej
requested changes
Mar 26, 2025
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
| const slideTimestamp = editingElement.addEventListener("slid.bs.carousel", () => { | ||
| const slideTimestamp = window.performance.now(); | ||
| // overlay should be handled by optionContainer | ||
| // setTimeout(() => this.trigger_up("hide_overlay")); |
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| // rewrite the jquery interface in carousel.js | ||
| const carouselInstance = window.Carousel.getOrCreateInstance(editingElement); |
There was a problem hiding this comment.
Watch out that you need to get the iframe's window, here you take the global window
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
FrancoisGe
force-pushed
the
master-mysterious-egg
branch
from
f468b7f to
80b3d5d
Compare
Jinjiu96
force-pushed
the
master-mysterious-egg-jili-1
branch
from
8d9a9cf to
cf055d0
Compare
ged-odoo
force-pushed
the
master-mysterious-egg
branch
from
27a1402 to
8c6b756
Compare
Jinjiu96
force-pushed
the
master-mysterious-egg-jili-1
branch
7 times, most recently
from
dda2df3 to
553226c
Compare
robinlej
reviewed
Mar 28, 2025
There was a problem hiding this comment.
I think there is some work left, many features still don't work.
- history (undo / redo) after sliding or adding a slide
- addSlide doesn't trigger a sliding animation
- change indicators type then click doesn't work
- change the arrows then click doesn't work
- ...
But while some are due to the PR, some functionalities don't work well in master either. The carousel is clearly very tricky and I'm not sure of what would be the best choices here. Even more so if we take into consideration that the same classes are used in different snippets.
On another note, the Carousel public widget inherited from CarouselHandler, with the method _updateIndicatorAndActivateSnippet. I don't think it is in use anywhere now, shouldn't you reimplement it too? 🤔
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
addons/html_builder/static/src/plugins/carousel_option_plugin.js
Outdated
Show resolved
Hide resolved
Jinjiu96
force-pushed
the
master-mysterious-egg-jili-1
branch
7 times, most recently
from
484d846 to
51195fe
Compare
divy-odoo
force-pushed
the
master-mysterious-egg
branch
from
4648461 to
2d7aa09
Compare
Jinjiu96
force-pushed
the
master-mysterious-egg-jili-1
branch
2 times, most recently
from
8dd80e2 to
1ccbada
Compare
Jinjiu96
force-pushed
the
master-mysterious-egg-jili-1
branch
from
8fcbd86 to
821aec5
Compare
FrancoisGe
reviewed
Apr 10, 2025
| @@ -122,6 +124,7 @@ export class BuilderNumberInput extends Component { | |||
| } | |||
| const unit = this.props.unit; | |||
| const saveUnit = this.props.saveUnit; | |||
| const withUnit = this.props.withUnit; | |||
There was a problem hiding this comment.
why do you need withUnit ? we don t need it in the old api
| step="0.1" | ||
| min="0" | ||
| preview="false" | ||
| withUnit="false"/> |
Jinjiu96
force-pushed
the
master-mysterious-egg-jili-1
branch
from
821aec5 to
770dd88
Compare
Goaman
force-pushed
the
master-mysterious-egg-jili-1
branch
from
770dd88 to
95fe961
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.