Skip to content

Add header_actions helper and split header partial - #85

Merged
thomasgallagher merged 2 commits into
mainfrom
header-helper-and-actions
May 18, 2026
Merged

Add header_actions helper and split header partial#85
thomasgallagher merged 2 commits into
mainfrom
header-helper-and-actions

Conversation

@thomasgallagher

Copy link
Copy Markdown
Collaborator

Extracts theme toggle, navigation toggle, and authentication into their own partials, and adds an l_ui_header_actions helper so host apps can append or replace the default header actions.

Extracts theme toggle, navigation toggle, and authentication into their
own partials, and adds an l_ui_header_actions helper so host apps can
append or replace the default header actions.
@claude

claude Bot commented May 18, 2026

Copy link
Copy Markdown

Review: Add header_actions helper and split header partial

Splits the monolithic header partial into three focused sub-partials (, , ) and exposes public helpers for each, plus three new yields (, , ) for host apps to compose the header action area.

Issues

Severity File Line(s) Description
MEDIUM 17, 27 yield(:l_ui_header_actions_start) and yield(:l_ui_header_actions_end) are rendered outside the if/else block, so they render unconditionally - even when :l_ui_header_actions is set to replace the defaults. A host app that sets :l_ui_header_actions_start in a parent layout and later uses :l_ui_header_actions in a child view will see both the _start content and the replaced actions rendered together, with no way to suppress the outer yields. This interaction is undocumented and easy to hit in multi-layout apps. Consider skipping the outer yields when content_for?(:l_ui_header_actions) is true, or document the behaviour explicitly.

Verdict

🟧 Ship (with known minor issues) - The core refactor is clean and well-tested; the only rough edge is the undocumented interaction between :l_ui_header_actions and the _start/_end yields.

@claude

claude Bot commented May 18, 2026

Copy link
Copy Markdown

Review: Add header_actions helper and split header partial

Splits the monolithic header partial into three focused sub-partials (_theme_toggle, _authentication, _navigation_toggle) and exposes public helpers for each, plus three new yields (:l_ui_header_actions, :l_ui_header_actions_start, :l_ui_header_actions_end) for host apps to compose the header action area.

Issues

Severity File Line(s) Description
MEDIUM app/views/layouts/layered_ui/_header.html.erb 17, 27 yield(:l_ui_header_actions_start) and yield(:l_ui_header_actions_end) are rendered outside the if/else block, so they render unconditionally - even when :l_ui_header_actions is set to replace the defaults. A host app that sets :l_ui_header_actions_start in a parent layout and later uses :l_ui_header_actions in a child view will see both the _start content and the replaced actions rendered together, with no way to suppress the outer yields. This interaction is undocumented and easy to hit in multi-layout apps. Consider skipping the outer yields when content_for?(:l_ui_header_actions) is true, or document the behaviour explicitly.

Verdict

🟧 Ship (with known minor issues) - The core refactor is clean and well-tested; the only rough edge is the undocumented interaction between :l_ui_header_actions and the _start/_end yields.

Previously :l_ui_header_actions_start and :l_ui_header_actions_end
rendered unconditionally, so a parent layout setting them combined
with a child view replacing :l_ui_header_actions produced both. Now
"replace" means replace the entire actions group.
@claude

claude Bot commented May 18, 2026

Copy link
Copy Markdown

Review

Extracts the theme toggle, authentication buttons, and navigation toggle out of _header.html.erb into their own partials, then surfaces them as l_ui_theme_toggle, l_ui_authentication, and l_ui_navigation_toggle helpers. Adds three header-action yield points (:l_ui_header_actions_start, :l_ui_header_actions_end, :l_ui_header_actions) so host apps can compose or replace the default action bar without overriding the full partial.

No issues found.

Ship

@thomasgallagher
thomasgallagher merged commit a94c32e into main May 18, 2026
4 checks passed
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.

1 participant