Skip to content

Fix side box positions in page form - #4684

Open
MizukiTemma wants to merge 3 commits into
developfrom
story/fix_settings_box_placement
Open

MizukiTemma wants to merge 3 commits into
developfrom
story/fix_settings_box_placement

Conversation

@MizukiTemma

@MizukiTemma MizukiTemma commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

Short description

This PR fixes the position of side boxes in the page form

Proposed changes

  • Check whether left column and right column are side by side or vertically placed, and move the box "Implications to translations" to match the design

Side effects

  • should be none

Faithfulness to issue description and design

There are no intended deviations from the issue and design.

How to test

  • Open a new page or an existing page
  • Change screen size
  • Check the position is as specified in the issue

Resolved issues

Fixes: #4651


Pull Request Review Guidelines

@MizukiTemma
MizukiTemma force-pushed the story/fix_settings_box_placement branch from 7d80068 to 51036c5 Compare November 3, 2026 16:57
@deliverino

deliverino Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

LLM PR Review (verdigado-code)

Analyzed diff, but not the whole repository. Important context might be missing.

Scope: The PR matches its description – it reorganises the sidebar columns in page_form.html for the new/existing page cases and adds JS logic to reposition the "Implications to translations" box. No unrelated changes detected.


Potential bug – pageTranslationBox likely resolves to the left column itself
(distribute-sidebar-boxes.ts, line 57):

const pageTranslationBox = document.getElementById("page-minor-edit")?.parentElement;

The include {% include "./page_form_sidebar/minor_edit_box.html" with box_id="page-minor-edit" %} most likely renders a single top-level element carrying id="page-minor-edit", making its parentElement the #left-sidebar-column div. If so, distributeBoxes would call rightColumn.prepend(leftColumn) (moving the entire left column, including the settings box, into the right column) or leftColumn.prepend(leftColumn) (a DOM no-op) depending on the layout branch. The .parentElement should probably be removed so that the actual box element is moved, or the included template must wrap the box in an additional container. This needs verification against page_form_sidebar/minor_edit_box.html.


No tests for new JS functions

isSideBySide and distributeBoxes are new, non-trivial functions with no unit tests. The isSideBySide heuristic (comparing getBoundingClientRect top/bottom) and the prepend logic on resize are the kind of code that benefits from tests, especially since the behaviour changes across viewport breakpoints.


Resize listener without debounce
(distribute-sidebar-boxes.ts, lines 75–77):

distributeBoxes is invoked on every single resize event. Each call performs two getBoundingClientRect() reads (forcing layout) and a potential DOM mutation. A short debounce (or requestAnimationFrame throttling) would avoid redundant work during continuous resizes.


Unnecessary resize listener in the fallback path
(distribute-sidebar-boxes.ts, lines 70–77):

The else branch is entered not only when pageTranslationBox exists, but also when it is falsy and one of the columns is missing. In that case distributeBoxes is a no-op (all three guards fail), yet a permanent resize listener is still registered. Guarding the addEventListener("resize", …) call on pageTranslationBox && leftColumn && rightColumn would avoid the dead listener.


Template: HIX widget column change for new pages
(page_form.html, lines 197–207):

For the not page case the HIX widget moves from the left column to the right column, and 4xl:flex is removed from the container. Combined with the JS distribution algorithm still running (since page-minor-edit is absent), the settings box and HIX widget will be redistributed by height. Verify this matches the intended design for new pages – the HIX widget may end up in the left column after redistribution.

@MizukiTemma
MizukiTemma marked this pull request as draft November 4, 2026 10:32
@MizukiTemma
MizukiTemma force-pushed the story/fix_settings_box_placement branch from a4a719a to 3ab32c2 Compare November 4, 2026 15:23
@MizukiTemma
MizukiTemma marked this pull request as ready for review November 4, 2026 16:09
@MizukiTemma
MizukiTemma force-pushed the story/fix_settings_box_placement branch from db9fd19 to b31bf4d Compare November 4, 2026 16:19

@dasgoutam dasgoutam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me:) Order of boxes appear as per the issue description

@jarlhengstmengel
jarlhengstmengel self-requested a review November 10, 2026 12:41

@jarlhengstmengel jarlhengstmengel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR 🚀 I have one point for when creating a new page.

// Check if there are multiple columns in the document
if (leftColumn && rightColumn) {

if (!pageTranslationBox && leftColumn && rightColumn) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checking for the page translationsbox creates the sitatuation that when creating a new page, the box isn't there and this block is entered. That results in that the settings box and the understandability box are sorted one over the other on a 4k/big screen like all boxes before. I know thats how it was in the Issue description but I feel like it would be better to have the two boxes in parallel like for the other cases. Maybe a question for @m0ndra?

Image

Co-authored-by: Jarl Hengstmengel <139335461+jarlhengstmengel@users.noreply.github.com>

This branch has not been deployed

No deployments
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.

Fix settings box placement when editing a page

3 participants