Skip to content

Match editor toolbar colours to Cacti themes and keep pickers visible - #290

Merged
TheWitness merged 16 commits into
Cacti:developfrom
alcatron:fix/editor-theme-controls
Oct 10, 2026
Merged

TheWitness merged 16 commits into
Cacti:developfrom
alcatron:fix/editor-theme-controls

Conversation

@alcatron

@alcatron alcatron commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

The editor toolbar used fixed blue colours and could have unreadable text when styled by Cacti’s theme. Use the theme’s title-row background with readable toolbar text, borders and hover states. Allow the toolbar height to follow its content.

Append autocomplete results to the active editor dialog so interface and graph pickers remain visible above the dialog. Keep enabled dialog actions readable.

Validation: all 225 plugin tests passed on PHP 8.3 in an isolated fixture installation; syntax and whitespace checks passed. The combined local Chrome audit checked the toolbar and picker layering across seven installed themes and both current/legacy dialog CSS paths. Full installation and cross-browser integration remain unverified for this branch.

Latest review follow-up

Removed the forced button text colour so native theme rules apply. Added a real jQuery UI regression invoking show_dialog() and checking autocomplete menu ownership on opening/reopening. Latest local validation: 225 PHP tests (5414 assertions), existing editor workflow and new picker-layering JavaScript checks, plus headless Chrome checks of native button text colours and menu ownership across ten isolated theme fixtures.

These follow-up checks use isolated local fixtures, not a running Cacti installation. PHP 8.3 syntax, whitespace, manifest and translation-template checks passed. GitHub CI must still confirm the revised commits.

Further review follow-up

Addressed in 1a9dc4d. Changed the toolbar to sticky positioning in normal page flow, contained its floated list with display: flow-root, and cleared the theme title-row float. Removed the fixed body/form offsets. Isolated Chrome fixtures using the actual editor toolbar markup/CSS and ten installed theme stylesheets passed at 320, 760 and 1400px widths: the form remains below the toolbar initially and after expanded help text, and the toolbar stays at the top when scrolling (90 checks). All 225 plugin tests pass (5414 assertions). This is fixture validation, not a full running-installation browser audit.

Additional review validation

Addressed the concerns in the latest Copilot review summary in 3547134. Added a scoped wm-editor-dialog class when the editor opens a dialog and overflow: visible on that wrapper. Autocomplete menus remain in the wrapper for stacking but can extend beyond its edges. The real jQuery UI ownership/reopen test passes, as do all 225 plugin tests (5414 assertions). In isolated Chrome, actual jQuery UI menus were tested across ten installed themes: a hidden-overflow rule reproduced clipping before the fix, while the fixed hidden-overflow and native-theme cases allowed hit-testing below the wrapper (30 fixtures). Menus were held open in these layout fixtures to avoid cross-frame focus closing them. This is not a complete live-installation browser audit.

@TheWitness
TheWitness requested review from TheWitness and a balanced review from Copilot and removed request for Copilot October 9, 2026 15:11

Copilot AI 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.

🟡 Changes recommended

The enabled-button fallback color fails accessible contrast on supported dark Cacti themes, and picker layering lacks regression coverage.

2 open findings
What changed in this PR

Updates the editor UI to better follow Cacti themes and improve picker layering.

Changes:

  • Applies themed toolbar styling and responsive height.
  • Keeps autocomplete results within active dialogs.
  • Improves dialog action visibility and documents the change.
File Description
weathermap-cacti-plugin-editor.php Adds the Cacti title-row theme class.
js/​editor.js Reparents autocomplete menus into dialogs.
css/​editor.css Adds theme-aware toolbar and dialog styling.
CHANGELOG.md Records issue #290.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread css/editor.css Outdated
Comment thread js/editor.js

Copilot AI 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.

🔵 Needs a closer look

The auto-height toolbar collapses around its floated child, preventing the themed background from covering the controls.

0 open findings

2 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Toolbar collapses because floated menu is not contained

css/​editor.css:47

height: auto does not make the toolbar follow its controls because its only child, #toolbar ul, remains floated. The toolbar therefore collapses to zero content height, so the theme background applied by cactiTableTitleRow is painted only behind the border instead of behind the transparent menu items. Establish a new block formatting context so the floated list contributes to the toolbar height.

🧠 Review effort: Balanced

@alcatron

Copy link
Copy Markdown
Contributor Author

I checked the toolbar-collapse concern from the latest review summary in isolated headless Chrome fixtures using the actual editor CSS and all ten installed Cacti theme stylesheets. The existing fixed-position toolbar already contains its floated list: its measured height is 40px for dark/modern and 52px for the other eight themes, with the list fully contained in every case. Adding display: flow-root produced identical geometry in all ten fixtures. I therefore left the CSS unchanged rather than adding an unverified fix. This verifies the fixture layout, not a complete running-installation browser/theme audit; if a specific installation still shows collapse, a reproduction would help identify the differing rule.

Copilot AI 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.

🟡 Changes recommended

A wrapped content-height toolbar can obscure the map because the underlying page retains a fixed offset.

1 open finding

🧠 Review effort: Balanced

Comment thread css/editor.css

Copilot AI 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.

🔵 Needs a closer look

Autocomplete menus can be clipped by themes that apply hidden overflow to dialogs.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Autocomplete menu is clipped by dialog overflow

js/​editor.js:1169

Appending the 300px autocomplete menu inside .ui-dialog makes it subject to the dialog's clipping. Current Cacti themes such as dark, modern, sunrise, and deepness set .ui-dialog { overflow: hidden; }, so a picker near the bottom of the node/link dialog can have most of its results cut off. The new jsdom test only checks DOM ownership and cannot detect this layout failure. Please either override overflow for these editor dialogs or keep the menu outside the clipped wrapper and raise its stacking order.

🧠 Review effort: Balanced

@alcatron

Copy link
Copy Markdown
Contributor Author

Addressed the concerns in the latest Copilot review summary in 3547134. Added a scoped wm-editor-dialog class when the editor opens a dialog and overflow: visible on that wrapper. Autocomplete menus remain in the wrapper for stacking but can extend beyond its edges. The real jQuery UI ownership/reopen test passes, as do all 225 plugin tests (5414 assertions). In isolated Chrome, actual jQuery UI menus were tested across ten installed themes: a hidden-overflow rule reproduced clipping before the fix, while the fixed hidden-overflow and native-theme cases allowed hit-testing below the wrapper (30 fixtures). Menus were held open in these layout fixtures to avoid cross-frame focus closing them. This is not a complete live-installation browser audit.

Copilot AI 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.

🔵 Needs a closer look

Theme-dependent layout and dialog layering still require live-installation cross-browser verification.

0 open findings

🧠 Review effort: Balanced

TheWitness
TheWitness previously approved these changes Oct 10, 2026
@alcatron

Copy link
Copy Markdown
Contributor Author

Resolved the CHANGELOG.md conflict with current develop in 781fe22, retaining one issue#290 entry. The editor theme and picker fixes remain intact. All 229 local PHP tests pass (5438 assertions), along with the real jQuery UI picker reopening regression, editor workflow check, manifest and whitespace checks. These are isolated fixture checks; full-installation cross-browser verification remains outstanding.

TheWitness
TheWitness previously approved these changes Oct 10, 2026
@alcatron

Copy link
Copy Markdown
Contributor Author

Resolved the new CHANGELOG.md conflict after #298 merged into develop. Both changelog entries are preserved; the editor theme and picker changes remain intact. All 230 local tests pass (5461 assertions), and the real jQuery UI picker reopening regression, editor syntax, manifest and whitespace checks pass.

@TheWitness
TheWitness merged commit 0ed19de into Cacti:develop Oct 10, 2026
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