Skip to content
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

ScrollButton #4195

Open
wants to merge 1 commit into
base: master-mysterious-egg
Choose a base branch
from
Open

Conversation

blse-odoo
Copy link

@blse-odoo blse-odoo commented Mar 14, 2025

No description provided.

@robodoo
Copy link

robodoo commented Mar 14, 2025

This PR targets the un-managed branch odoo-dev/odoo:master-mysterious-egg, it needs to be retargeted before it can be merged.

@blse-odoo blse-odoo force-pushed the master-mysterious-egg-3-blse branch 2 times, most recently from ee7c7b0 to 26d95b3 Compare March 17, 2025 08:19
@blse-odoo blse-odoo force-pushed the master-mysterious-egg-3-blse branch 2 times, most recently from b5078a7 to cd469ad Compare March 17, 2025 10:23
import { _t } from "@web/core/l10n/translation";
import { registry } from "@web/core/registry";
import { ScrollButtonOption } from "./scroll_button_option";
import { classAction } from "@html_builder/core/plugins/core_builder_action_plugin";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably ok here as we already do it a lot in the code but I tell you so that you know for the next time: the correct way to do this is to use getActionId() ; that way, if classAction is redefined in website we will have the correct version of it 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I indeed found a lot of examples using classAction directly. I'm ok to do it differently, but I did not find any getActionId (maybe this is another name?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is getAction() 👍 If you are inside a plugin, you can use this.dependencies.builderActions.getAction by adding builderActions in the dependencies of the plugin

}
}
}
registry.category("website-plugins").add(ScrollButtonOptionPlugin.id, ScrollButtonOptionPlugin);
Copy link

@loco-odoo loco-odoo Mar 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked functionally and it seems that there is a problem; if you put the height of the snippet at 100% and then go back to "auto' there is a big blank space remaining, maybe worth having a look 👍
Edit: also note that your runbot is yellow because ScrollButtonOption misses static props description (here static props = {};)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed 2 bugs that look like what you are describing:

  • If we set 100%, then something else, it happens often that the "min-height" on the section set by the fullscreen interaction stays in the dom, and keeps it fullscreen, with blanks around the gallery
  • If we set a big height for the gallery (for example by setting 100%), then a smaller height, the interaction ensuring that each element of the gallery has the same height as the tallest one in the gallery keep enforcing the height as before the reduction of size.

Is seems to be the same underlying bug related to how interactions are not properly cleaned when saving a step or something like that. (I have no idea how to fix that)

@blse-odoo blse-odoo force-pushed the master-mysterious-egg-3-blse branch from cd469ad to c2f9246 Compare March 18, 2025 09:28
@FrancoisGe FrancoisGe force-pushed the master-mysterious-egg branch from 3ee87bf to 98b990a Compare March 19, 2025 09:22
@blse-odoo blse-odoo force-pushed the master-mysterious-egg-3-blse branch 3 times, most recently from 0467eae to 664ae6d Compare March 24, 2025 12:30
@blse-odoo blse-odoo force-pushed the master-mysterious-egg-3-blse branch from 664ae6d to c67043e Compare March 24, 2025 14:11
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.

3 participants