Skip to content

Conversation

@dabo-odoo
Copy link
Contributor

@dabo-odoo dabo-odoo commented Mar 11, 2025

These commits fix two issues:

  • If a block that has an image selected as background is given a
    border, its background image will fit awkwardly in the surrounding
    border, and similarly if the background image is positioned all the
    way to any edge, it does not cover all the block.
  • If a video is selected as background for a block, the video does not
    always fill the block.

Steps to reproduce:

Bug 1

  • Add a masonry block/big boxes block
  • Add a background image to an element of the masonry/big boxes
  • Add a big border
  • Make the border translucent if it isn't already to better see the bug
    => The background image overflows on the border randomly
  • Change the background position by shifting the image all the way to
    the left
    => The image does not cover all the border-box

Bug 2

  • Add a masonry block/big boxes block
  • Add a video background
    => The video does not always fill the box, especially when the window
    gets resized

After the changes the image/video background completely covers its
block, even when resized or when a border is applied.

task-4174638

@robodoo
Copy link
Contributor

robodoo commented Mar 11, 2025

Pull request status dashboard

@C3POdoo C3POdoo added the RD research & development, internal work label Mar 11, 2025
@dabo-odoo dabo-odoo force-pushed the 16.0-website-masonry-background-image-border-layer-fix-dabo branch 2 times, most recently from aac7773 to 441cf4f Compare March 13, 2025 12:20
@dabo-odoo dabo-odoo changed the title [FIX] website: masonry background image border layer fix [FIX] website, web_editor: masonry background image border layer fix Mar 13, 2025
@dabo-odoo dabo-odoo force-pushed the 16.0-website-masonry-background-image-border-layer-fix-dabo branch from 441cf4f to fc13ca1 Compare March 13, 2025 12:30
@dabo-odoo dabo-odoo changed the title [FIX] website, web_editor: masonry background image border layer fix [FIX] website, web_editor: block background video/image border layer fix Mar 13, 2025
@dabo-odoo dabo-odoo force-pushed the 16.0-website-masonry-background-image-border-layer-fix-dabo branch 2 times, most recently from b6d5f4a to 189680f Compare March 13, 2025 14:47
Copy link
Contributor

@qsm-odoo qsm-odoo left a comment

Choose a reason for hiding this comment

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

Multiples bugs: can't there be multiple commits please? 🙏

Comment on lines 407 to 409
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of those CSS rules duplicated? 🤔 Can't they not be? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the duplicate

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you did: I think you need a third commit (to be the first one of your PR), to remove the duplication. The CSS rules for o_bg_img_opt_repeat, o_bg_img_center, ... seems to be duplicated for no reason: they should be in web_editor only.

Then you add a new one in this commit, which should also be defined in web_editor, not website since your JS code is using it in web_editor, not website 👍

Comment on lines 305 to 307
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Again: duplicated CSS rules -> no
  • I don't want this class, the border class of Bootstrap should be enough: why is it not? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The border class of bootstrap takes $border-color which is transparent, so I thought to override it with a custom class that takes currentColor as parameter instead: this also has the benefit of setting a default border color that changes according to the chosen theme instead of the default gray-300.
Will remove the duplicates

Copy link
Contributor

Choose a reason for hiding this comment

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

The border class of bootstrap takes $border-color which is transparent

I don't think it ever was no (you are saying yourself: "instead of the default gray-300") 🤔 Plus I just retested in 16.0 and 18.0, I don't have that transparent bug anymore somehow 🤷 Could you double check?

so I thought to override it with a custom class that takes currentColor as parameter instead

It's already the default behavior of the border class in at least 18.0 😉 And since in 16.0 it does not seem buggy anymore (or did I test it wrong?), I don't think we need to change the behavior of the option, could you double check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked again and the default transparent bug is present in 17 and 18 but not in 16
16
17
17-4
18

@dabo-odoo dabo-odoo force-pushed the 16.0-website-masonry-background-image-border-layer-fix-dabo branch from 189680f to ca2ed58 Compare March 18, 2025 10:24
@dabo-odoo
Copy link
Contributor Author

dabo-odoo commented Mar 18, 2025

@qsm-odoo Reorganized and split the PR into two commits, one for the background image bug, one for the background video bug

Copy link
Contributor

@qsm-odoo qsm-odoo left a comment

Choose a reason for hiding this comment

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

Ping me once latest adaptations done :)

Comment on lines 407 to 409
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you did: I think you need a third commit (to be the first one of your PR), to remove the duplication. The CSS rules for o_bg_img_opt_repeat, o_bg_img_center, ... seems to be duplicated for no reason: they should be in web_editor only.

Then you add a new one in this commit, which should also be defined in web_editor, not website since your JS code is using it in web_editor, not website 👍

@dabo-odoo dabo-odoo force-pushed the 16.0-website-masonry-background-image-border-layer-fix-dabo branch from ca2ed58 to 9048a1d Compare March 26, 2025 14:17
@dabo-odoo
Copy link
Contributor Author

@qsm-odoo Added a third commit removing the duplicate classes and split the comment to respect the 80 characters limit

This commit fixes issues with the border of blocks that have images as
background.
- If a block that has an image selected as background is given a
  border, its background image will fit awkwardly in the surrounding
  border.
- If a backround image for a block is positioned all the way to any
  edge, it does not cover all the block.

Steps to reproduce:

- Add a masonry block/big boxes block
- Add a background image to an element of the masonry/big boxes
- Add a big border
=> The background image overflows on the border randomly
- Make the border translucent if it isn't already
- Shift the background image all the way to the left/top
=> The image does not cover all the border/box 

After the change the image background completely covers its
block, even when resized or when a border is applied.

task-4174638
If a video is selected as background for a block, the video does not
always fill the block, especially when editing and resizing the
block.

Steps to reproduce:
- Add a masonry block/big boxes block
- Add a video background
- Resize the block
=> The video does not always fill the box, especially when the window
gets resized

After the change video background completely covers its block, even
when resized.

task-4174638
@qsm-odoo qsm-odoo force-pushed the 16.0-website-masonry-background-image-border-layer-fix-dabo branch from 9048a1d to b521ff1 Compare March 26, 2025 15:48
@qsm-odoo qsm-odoo marked this pull request as ready for review March 26, 2025 15:49
@C3POdoo C3POdoo requested a review from a team March 26, 2025 15:51
Copy link
Contributor

@qsm-odoo qsm-odoo left a comment

Choose a reason for hiding this comment

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

@dabo-odoo In fact, my bad, we don't remove dead code (duplicated CSS here) in stable. So I removed the first commit and I am opening a new PR with it for master 👍

Let's move forward with the rest 💪

@robodoo rebase-ff r+

@robodoo
Copy link
Contributor

robodoo commented Mar 26, 2025

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request Mar 26, 2025
This commit fixes issues with the border of blocks that have images as
background.
- If a block that has an image selected as background is given a
  border, its background image will fit awkwardly in the surrounding
  border.
- If a backround image for a block is positioned all the way to any
  edge, it does not cover all the block.

Steps to reproduce:

- Add a masonry block/big boxes block
- Add a background image to an element of the masonry/big boxes
- Add a big border
=> The background image overflows on the border randomly
- Make the border translucent if it isn't already
- Shift the background image all the way to the left/top
=> The image does not cover all the border/box 

After the change the image background completely covers its
block, even when resized or when a border is applied.

task-4174638

Part-of: #201200
Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
robodoo pushed a commit that referenced this pull request Mar 26, 2025
If a video is selected as background for a block, the video does not
always fill the block, especially when editing and resizing the
block.

Steps to reproduce:
- Add a masonry block/big boxes block
- Add a video background
- Resize the block
=> The video does not always fill the box, especially when the window
gets resized

After the change video background completely covers its block, even
when resized.

task-4174638

closes #201200

Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
@robodoo robodoo closed this Mar 26, 2025
@qsm-odoo qsm-odoo deleted the 16.0-website-masonry-background-image-border-layer-fix-dabo branch March 27, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RD research & development, internal work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants