-
Notifications
You must be signed in to change notification settings - Fork 116
carousel: select first slide on save #4575
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
Conversation
This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg, it needs to be retargeted before it can be merged. |
clean_for_save_handlers: ({ root }) => { | ||
for (const carouselEl of root.querySelectorAll(".carousel")) { | ||
this.slide(carouselEl, 0); | ||
} | ||
}, |
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.
I m not sure that is a good idea to call slide. Slide is async, it will do a setTimeout to apply the active class
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.
I dont know if we just need to move the active class to the first slide or if we need to update some other stuff with bootstrap
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.
I used before_save_handlers
instead, which is awaited for.
Too bad we do not have a naming convention to easily differentiate them (sync vs async).
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.
In the old website, we are actually restoring the first slide by setting the classes. I think that's a bit better than doing the sliding call. The slide
function is called when editing because we want that carousel sliding animation effect. But on save, we don't care much how we slide to the first slide so we can just set the classes instead
_restoreCarousels() {
this.$editable[0].querySelectorAll(".carousel").forEach(carouselEl => {
// Set the first slide as the active one.
carouselEl.querySelectorAll(".carousel-item").forEach((itemEl, i) => {
itemEl.classList.remove("next", "prev", "left", "right");
itemEl.classList.toggle("active", i === 0);
});
carouselEl.querySelectorAll(".carousel-indicators > *").forEach((indicatorEl, i) => {
indicatorEl.classList.toggle("active", i === 0);
indicatorEl.removeAttribute("aria-current");
if (i === 0) {
indicatorEl.setAttribute("aria-current", "true");
}
});
});
}
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.
I disagree: this save side effect impacts the content of the page. Actually showing the slide effect helps attract the attention of the user to notice that something is happening without being explicitly requested.
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.
I meant we don't care much cause when we click save, there's a gray semi-transparent overlay on the whole website editing page. I think the overlay already gives the info that the website is being modified and saved. 🤔
I think it's more a PO decision so I'll ping @lebl-odoo , the question is: shall we have the sliding effect when setting the carousel back to the first slide on save? 🙏
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.
As we had in the discussion, the worrying risk (slide never finishes and website stuck) is in carousel_slider.js
, and SEBA is fixing the problem on stable. Also if the slide
is broken, it should probably break already before we click save, e.g. when we add a slide or simply click on control buttons. The calling of the slide
itself should be considered as safe.
ab7c5f0
to
d759fb0
Compare
pre-forward port of 208853
d759fb0
to
0b233fb
Compare
No description provided.