Skip to content

Commit

Permalink
The connectedCallback of the controller sometimes fires too early, re…
Browse files Browse the repository at this point in the history
…sultig in the button not shrinking. So instead, a mobile alternative is rendered for each button, which already has the :small size (#108)
  • Loading branch information
HDinger authored Apr 26, 2024
1 parent 4c5ccf2 commit 22cf2ef
Show file tree
Hide file tree
Showing 12 changed files with 81 additions and 70 deletions.
5 changes: 5 additions & 0 deletions .changeset/few-teachers-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@openproject/primer-view-components": patch
---

Show single actions of Primer::OpenProject::PageHeader in a smaller variant on mobile
2 changes: 2 additions & 0 deletions app/components/primer/open_project/page_header.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
<% menu.with_show_button(icon: :"kebab-horizontal", size: :small, "aria-label": @mobile_menu_label) %>
<% @desktop_menu_block.call(menu) unless @desktop_menu_block.nil? %>
<% end %>
<% elsif actions.length == 1 && @mobile_action.present? %>
<%= render(@mobile_action) { |el| @mobile_action_block.call(el) unless @mobile_action_block.nil?} %>
<% end %>
</div>
<% end %>
Expand Down
10 changes: 0 additions & 10 deletions app/components/primer/open_project/page_header.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,6 @@
align-items: center;
}

.PageHeader--singleAction .PageHeader-action {
@media (max-width: 543.98px) {
position: absolute;
top: 10px;

/* Normally, the actions are hidden on mobile, except for this special case of a single action */
display: flex !important;
}
}

.PageHeader-breadcrumbs {
display: block;
width: 100%;
Expand Down
89 changes: 53 additions & 36 deletions app/components/primer/open_project/page_header.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class PageHeader < Primer::Component
].freeze

DEFAULT_ACTION_SCHEME = :default
MORE_MENU_DISPLAY = [:flex, :none].freeze
MOBILE_ACTIONS_DISPLAY = [:flex, :none].freeze

DEFAULT_LEADING_ACTION_DISPLAY = [:none, :flex].freeze
DEFAULT_BREADCRUMBS_DISPLAY = [:none, :flex].freeze
Expand Down Expand Up @@ -58,32 +58,37 @@ class PageHeader < Primer::Component
#
# @param system_arguments [Hash] <%= link_to_system_arguments_docs %>
renders_many :actions, types: {
icon_button: lambda { |icon:, mobile_icon:, label:, scheme: DEFAULT_ACTION_SCHEME, **system_arguments|
icon_button: lambda { |icon:, mobile_icon:, label:, scheme: DEFAULT_ACTION_SCHEME, **system_arguments, &block|
deny_tag_argument(**system_arguments)

system_arguments = set_action_arguments(system_arguments, scheme: scheme, button_action: true)
system_arguments[:icon] = icon
system_arguments[:"aria-label"] ||= label
system_arguments = set_action_arguments(system_arguments, scheme: scheme)

add_option_to_mobile_menu(system_arguments, mobile_icon, label, scheme)
component = Primer::Beta::IconButton
create_mobile_alternatives(component, mobile_icon, label, scheme, **system_arguments, &block)

Primer::Beta::IconButton.new(icon: icon, "aria-label": label, **system_arguments)
component.new(**system_arguments)
},
button: lambda { |mobile_icon:, mobile_label:, scheme: DEFAULT_ACTION_SCHEME, **system_arguments|
button: lambda { |mobile_icon:, mobile_label:, scheme: DEFAULT_ACTION_SCHEME, **system_arguments, &block|
deny_tag_argument(**system_arguments)

system_arguments = set_action_arguments(system_arguments, scheme: scheme, button_action: true)
system_arguments = set_action_arguments(system_arguments, scheme: scheme)

add_option_to_mobile_menu(system_arguments, mobile_icon, mobile_label, scheme)
component = Primer::Beta::Button
create_mobile_alternatives(component, mobile_icon, mobile_label, scheme, **system_arguments, &block)

Primer::Beta::Button.new(**system_arguments)
component.new(**system_arguments)
},
zen_mode_button: lambda { |mobile_icon: Primer::OpenProject::ZenModeButton::ZEN_MODE_BUTTON_ICON, mobile_label: Primer::OpenProject::ZenModeButton::ZEN_MODE_BUTTON_LABEL, **system_arguments|
zen_mode_button: lambda { |mobile_icon: Primer::OpenProject::ZenModeButton::ZEN_MODE_BUTTON_ICON, mobile_label: Primer::OpenProject::ZenModeButton::ZEN_MODE_BUTTON_LABEL, **system_arguments, &block|
deny_tag_argument(**system_arguments)

system_arguments = set_action_arguments(system_arguments, scheme: DEFAULT_ACTION_SCHEME, button_action: true)
system_arguments = set_action_arguments(system_arguments, scheme: DEFAULT_ACTION_SCHEME)

add_option_to_mobile_menu(system_arguments, mobile_icon, mobile_label, DEFAULT_ACTION_SCHEME)
component = Primer::OpenProject::ZenModeButton
create_mobile_alternatives(component, mobile_icon, mobile_label, DEFAULT_ACTION_SCHEME, **system_arguments, &block)

Primer::OpenProject::ZenModeButton.new(**system_arguments)
component.new(**system_arguments)
},

link: lambda { |mobile_icon:, mobile_label:, scheme: DEFAULT_ACTION_SCHEME, **system_arguments|
Expand All @@ -109,29 +114,31 @@ class PageHeader < Primer::Component
renders: lambda { |**system_arguments, &block|
deny_tag_argument(**system_arguments)

system_arguments[:menu_arguments] = set_action_arguments(system_arguments[:menu_arguments])
system_arguments[:button_arguments] ||= {}
system_arguments[:button_arguments][:data] ||= {}
system_arguments[:button_arguments][:data][:targets] = "page-header.actionItems"
system_arguments[:button_arguments] = set_action_arguments(system_arguments[:button_arguments])

# Add the options individually to the mobile menu in the template
@desktop_menu_block = block

Primer::OpenProject::PageHeader::Menu.new(**system_arguments)
component = Primer::OpenProject::PageHeader::Menu
create_mobile_single_action(component, **system_arguments, &block)

component.new(**system_arguments)
},
},
dialog: {
renders: lambda { |mobile_icon:, mobile_label:, **system_arguments|
renders: lambda { |mobile_icon:, mobile_label:, **system_arguments, &block|
deny_tag_argument(**system_arguments)

# The id will be automatically calculated for the trigger button, so we have to behave the same, for the mobile click to work
system_arguments[:button_arguments] ||= {}
system_arguments[:button_arguments][:id] = "dialog-show-#{system_arguments[:dialog_arguments][:id]}"
system_arguments[:button_arguments] = set_action_arguments(system_arguments[:button_arguments], button_action: true)
system_arguments[:button_arguments] = set_action_arguments(system_arguments[:button_arguments])

add_option_to_mobile_menu(system_arguments[:button_arguments], mobile_icon, mobile_label, :default)
component = Primer::OpenProject::PageHeader::Dialog
create_mobile_alternatives(component, mobile_icon, mobile_label, :default, **system_arguments, &block)

Primer::OpenProject::PageHeader::Dialog.new(**system_arguments)
component.new(**system_arguments)
},
},
}
Expand Down Expand Up @@ -212,7 +219,7 @@ def initialize(mobile_menu_label: I18n.t("label_more"), **system_arguments)
)

@mobile_action_menu = Primer::Alpha::ActionMenu.new(
display: MORE_MENU_DISPLAY,
display: MOBILE_ACTIONS_DISPLAY,
anchor_align: :end
)
end
Expand All @@ -224,15 +231,6 @@ def render?
title? && breadcrumbs?
end

def before_render
@system_arguments[:classes] = class_names(
@system_arguments[:classes],
"PageHeader--singleAction": !render_mobile_menu?
)

content
end

def render_mobile_menu?
actions.count > 1
end
Expand All @@ -242,26 +240,32 @@ def render_mobile_menu?
def set_action_arguments(system_arguments, scheme: nil, button_action: false)
system_arguments[:ml] ||= 2
system_arguments[:display] = [:none, :flex]
system_arguments[:size] = :medium
system_arguments[:scheme] = scheme unless scheme.nil?
system_arguments[:classes] = class_names(
system_arguments[:classes],
"PageHeader-action",
)
if button_action
system_arguments[:data] ||= {}
system_arguments[:data][:targets] = "page-header.actionItems"
end

system_arguments[:id] ||= self.class.generate_id
system_arguments
end

def create_mobile_alternatives(component, mobile_icon, mobile_label, scheme, **system_arguments, &block)
# All actions should collapse into a single actionMenu on mobile
add_option_to_mobile_menu(system_arguments, mobile_icon, mobile_label, scheme)

# Except for single actions, which remain as they are, just smaller.
create_mobile_single_action(component, **system_arguments, &block)
end

def add_option_to_mobile_menu(system_arguments, mobile_icon, mobile_label, scheme)
unless mobile_icon.nil? || mobile_label.nil?
# In action menus, only :default and :danger are allowed
scheme = DEFAULT_ACTION_SCHEME unless scheme == :danger

with_menu_item(id: system_arguments[:id], label: mobile_label, scheme: scheme) do |c|
id = system_arguments[:button_arguments].present? ? system_arguments[:button_arguments][:id] : system_arguments[:id]
with_menu_item(id: id, label: mobile_label, scheme: scheme) do |c|
c.with_leading_visual_icon(icon: mobile_icon)
end
end
Expand All @@ -281,6 +285,19 @@ def with_menu_item(id:, **system_arguments, &block)
)
end

def create_mobile_single_action(component, **system_arguments, &block)
# Single actions shall not collapse into an action menu on mobile, but keep their state.
# However the position and size will change
unless render_mobile_menu?
mobile_options = system_arguments[:button_arguments].present? ?
{ button_arguments: { display: MOBILE_ACTIONS_DISPLAY, size: :small } } :
{ display: MOBILE_ACTIONS_DISPLAY, size: :small }

@mobile_action = component.new(**system_arguments.deep_merge(mobile_options))
@mobile_action_block = block
end
end

# transform anchor tag strings to {href, text} objects
# e.g "\u003ca href=\"/admin\"\u003eAdministration\u003c/a\u003e"
def anchor_string_to_object(html_string)
Expand Down
17 changes: 1 addition & 16 deletions app/components/primer/open_project/page_header_element.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,7 @@
import {controller, targets} from '@github/catalyst'
import {controller} from '@github/catalyst'

@controller
class PageHeaderElement extends HTMLElement {
@targets actionItems: HTMLElement[]

connectedCallback() {
for (const item of this.actionItems) {
/*
If there is only one action to be shown, we show that instead of the mobile action menu. However, the buttons should be the smaller button variant.
Unfortunately, the `size` attribute does not support responsive attributes and the .pcss syntax does not support inheritance between classes.
So we have to add the class manually here.
*/
if (window.innerWidth <= 544) {
item.classList.add('Button--small')
}
}
}

menuItemClick(event: Event) {
const currentTarget = event.currentTarget as HTMLButtonElement

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
scheme: :default,
id: "zenModeButton",
icon: ZEN_MODE_BUTTON_ICON,
size: @button_size,
aria: { label: ZEN_MODE_BUTTON_LABEL },
data: { target: "zen-mode-button.button", action: "click:zen-mode-button#performAction" }
)
Expand Down
2 changes: 2 additions & 0 deletions app/components/primer/open_project/zen_mode_button.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ def initialize(**system_arguments)
@system_arguments[:classes],
"ZenModeButton"
)

@button_size = @system_arguments[:size] || Primer::Beta::Button::DEFAULT_SIZE
end
end
end
Expand Down
15 changes: 15 additions & 0 deletions previews/primer/open_project/page_header_preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,21 @@ def zen_mode_button_actions
end
end


# @label With a single action
# The single action will not be transformed into a menu on mobile, but remains in a smaller variant
def single_action
render(Primer::OpenProject::PageHeader.new) do |component|
component.with_title { "Great news" }
component.with_breadcrumbs([{ href: "/foo", text: "Foo" }, { href: "/bar", text: "Bar" }, "Baz"])

component.with_action_button(mobile_icon: "plus", mobile_label: "Meeting", scheme: :primary) do |button|
button.with_leading_visual_icon(icon: "plus")
"Meeting"
end
end
end

# @label With leading action (on wide)
# **Leading action** is only shown on **wide screens** by default.
# If you want to override that behaviour please use the system_argument: **display**
Expand Down
3 changes: 0 additions & 3 deletions static/classes.json
Original file line number Diff line number Diff line change
Expand Up @@ -429,9 +429,6 @@
"PageHeader": [
"Primer::OpenProject::PageHeader"
],
"PageHeader--singleAction": [
"Primer::OpenProject::PageHeader"
],
"PageHeader-actions": [
"Primer::OpenProject::PageHeader"
],
Expand Down
2 changes: 1 addition & 1 deletion static/constants.json
Original file line number Diff line number Diff line change
Expand Up @@ -1463,7 +1463,7 @@
"h5",
"h6"
],
"MORE_MENU_DISPLAY": [
"MOBILE_ACTIONS_DISPLAY": [
"flex",
"none"
],
Expand Down
2 changes: 1 addition & 1 deletion test/components/primer/open_project/page_header_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ def test_renders_single_action
assert_selector(".PageHeader-title")
assert_text("An action")
assert_selector(".PageHeader-actions")
assert_selector(".PageHeader--singleAction")
assert_selector(".PageHeader-contextBar .Button--small.d-flex.d-sm-none")
end

def test_renders_leading_action
Expand Down
3 changes: 0 additions & 3 deletions test/css/component_specific_selectors_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,6 @@ class ComponentSpecificSelectorsTest < Minitest::Test
".InputGroup-input-width--large",
".InputGroup-input-width--xlarge",
".InputGroup-input-width--xxlarge",
],
Primer::OpenProject::PageHeader => [
".PageHeader--singleAction .PageHeader-action",
]
}.freeze

Expand Down

0 comments on commit 22cf2ef

Please sign in to comment.