N°2847 - Code clean up

* Fix TWIG exceptions due non existent JS templates for ajax tab * Fix DisplayBlock::GetRenderContent and HistoryBlock::GetRenderContent signature mismatch warning * Add return type hinting on ButtonFactory methods * Rename ButtonFactory::MakeAlternativeNeutralActionButton() to ButtonFactory::MakeLinkNeutral() * Add ButtonFactory::MakeLinkNeutral() to visual test page * Fix button spacing/padding when only icon or label
2026-04-21 09:38:48 +02:00 · 2020-09-28 17:47:27 +02:00
parent 2b0bdda1e0
commit 3dc7b66f6f
6 changed files with 107 additions and 86 deletions
--- a/application/displayblock.class.inc.php
+++ b/application/displayblock.class.inc.php
@@ -1367,7 +1367,7 @@ class HistoryBlock extends DisplayBlock
 		$this->iLimitCount = $iCount;
 	}

-	public function GetRenderContent(WebPage $oPage, $aExtraParams = array(), string $sId = null): iUIBlock
+	public function GetRenderContent(WebPage $oPage, array $aExtraParams = [], string $sId = null): iUIBlock
 	{
 		$sHtml = '';
 		$bTruncated = false;
@@ -1804,10 +1804,10 @@ class MenuBlock extends DisplayBlock
 			} else {
 				$sName = 'UI:Menu:Actions';
 			}
-			$oActionButton = ButtonFactory::MakeAlternativeNeutralActionButton('', $sName, 'fas fa-ellipsis-v', '', '', $sMenuTogglerId);
+			$oActionButton = ButtonFactory::MakeLinkNeutral('', '', 'fas fa-ellipsis-v', $sName, '', $sMenuTogglerId);
 			// TODO Add Js
-			$oActionsBlock->AddSubBlock($oActionButton);
-			$oActionsBlock->AddSubBlock($oPage->GetPopoverMenu($sPopoverMenuId, $aActions));
+			$oActionsBlock->AddSubBlock($oActionButton)
+				->AddSubBlock($oPage->GetPopoverMenu($sPopoverMenuId, $aActions));
 			$oActionButton->AddCSSClasses('ibo-action-button')
 				->SetJsCode(<<<JS
 $("#{$sPopoverMenuId}").popover_menu({toggler: "#{$sMenuTogglerId}"});
@@ -1829,7 +1829,7 @@ JS
 				);

 			if ($this->m_sStyle == 'details') {
-				$oActionButton = ButtonFactory::MakeAlternativeNeutralActionButton('', 'UI:SearchFor_Class', 'fas fa-search', "{$sRootUrl}pages/UI.php?operation=search_form&do_search=0&class=$sClass{$sContext}");
+				$oActionButton = ButtonFactory::MakeLinkNeutral("{$sRootUrl}pages/UI.php?operation=search_form&do_search=0&class=$sClass{$sContext}", '', 'fas fa-search', 'UI:SearchFor_Class');
 				$oActionButton->SetTooltip(Dict::Format('UI:SearchFor_Class', MetaModel::GetName($sClass)))
 					->AddCSSClasses('ibo-action-button');
 				$oActionsBlock->AddSubBlock($oActionButton);
@@ -1840,8 +1840,9 @@ JS
 				$sRefreshAction = "window.location.reload();";
 			}
 			if (!$oPage->IsPrintableVersion() && ($sRefreshAction != '')) {
-				$oActionButton = ButtonFactory::MakeAlternativeNeutralActionButton('', 'UI:Button:Refresh', 'fas fa-sync');
-				$oActionButton->SetOnClickJsCode($sRefreshAction)
+				$oActionButton = ButtonFactory::MakeAlternativeNeutral('', 'UI:Button:Refresh');
+				$oActionButton->SetIconClass('fas fa-sync')
+					->SetOnClickJsCode($sRefreshAction)
 					->SetTooltip(Dict::S('UI:Button:Refresh'))
 					->AddCSSClasses('ibo-action-button');
 				$oActionsBlock->AddSubBlock($oActionButton);
@@ -1882,7 +1883,7 @@ JS
 				}

 				$sTarget = isset($aAction['target']) ? $aAction['target'] : '';
-				$oActionButton = ButtonFactory::MakeAlternativeNeutralActionButton($sLabel, $sActionId, $sIconClass, $sUrl, $sTarget);
+				$oActionButton = ButtonFactory::MakeLinkNeutral($sUrl, $sLabel, $sIconClass, $sActionId, $sTarget);
 				$oActionButton->AddCSSClasses('ibo-action-button');
 				$oActionsBlock->AddSubBlock($oActionButton);
 			}
--- a/css/backoffice/components/_button.scss
+++ b/css/backoffice/components/_button.scss
@@ -1,10 +1,13 @@
+$ibo-button--sibling-spacing: 5px !default;
 $ibo-button--padding-y: 6px !default;
 $ibo-button--padding-x: 9px !default;
 $ibo-button--border: 0 !default;
 $ibo-button--border-radius: $ibo-border-radius-400 !default;
-
 $ibo-button--box-shadow-bottom: 0px 2px 0px !default;
 $ibo-button--box-shadow-top: inset 0px 2px 0px !default;
+
+$ibo-button--label--margin-left: 4px !default;
+
 $ibo-button-colors: (
 		'regular': (
 				'neutral': (
@@ -275,7 +278,6 @@ $ibo-button-colors: (
 				),
 		)
 ) !default;
-
@each $sType, $aColors in $ibo-button-colors {
 	@each $sColor, $aPseudoclasses in $aColors {
 		@each $sPseudoclass, $sAttributes in $aPseudoclasses {
@@ -291,17 +293,15 @@ $ibo-button-colors: (
 	}
 }

-$ibo-button-icon--padding-right: 4px !default;
-
 .ibo-button {
 	cursor: pointer;
 	@extend %ibo-font-ral-med-150;
 	padding: $ibo-button--padding-y $ibo-button--padding-x;
-	border: 0;
+	border: $ibo-button--border;
 	border-radius: $ibo-button--border-radius;

 	& ~ .ibo-button {
-		margin-left: 5px;
+		margin-left: $ibo-button--sibling-spacing;
 	}

 	&.ibo-action-button {
@@ -309,7 +309,8 @@ $ibo-button-icon--padding-right: 4px !default;
 	}
 }

-.ibo-button-icon {
-	padding-right: $ibo-button-icon--padding-right;
+/* Only when a button has both an icon and a label */
+.ibo-button-icon + .ibo-button-label {
+	margin-left: $ibo-button--label--margin-left;
 }

--- a/sources/application/UI/Component/Button/ButtonFactory.php
+++ b/sources/application/UI/Component/Button/ButtonFactory.php
@@ -29,6 +29,10 @@ namespace Combodo\iTop\Application\UI\Component\Button;
 */
 class ButtonFactory
 {
+	//---------------------------------------------
+	// Regular action buttons, mostly used in forms
+	//---------------------------------------------
+
 	/**
 	 * Make a basis Button component for any purpose
 	 *
@@ -38,8 +42,7 @@ class ButtonFactory
 	 *
 	 * @return \Combodo\iTop\Application\UI\Component\Button\Button
 	 */
-	public static function MakeNeutral(string $sLabel, string $sName, ?string $sId = null)
-	{
+	public static function MakeNeutral(string $sLabel, string $sName, ?string $sId = null): Button {
 		$oButton = new Button($sLabel, $sId);
 		$oButton->SetActionType(Button::ENUM_ACTION_TYPE_REGULAR)
 			->SetColor(Button::ENUM_COLOR_NEUTRAL)
@@ -65,7 +68,7 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_PRIMARY, Button::ENUM_ACTION_TYPE_REGULAR, $sValue, $sName, $bIsSubmit, $sId);
 	}

@@ -86,7 +89,7 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_SECONDARY, Button::ENUM_ACTION_TYPE_REGULAR, $sValue, $sName, $bIsSubmit, $sId);
 	}

@@ -108,7 +111,7 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_VALIDATION, Button::ENUM_ACTION_TYPE_REGULAR, $sValue, $sName, $bIsSubmit, $sId);
 	}

@@ -130,28 +133,35 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_DESTRUCTIVE, Button::ENUM_ACTION_TYPE_REGULAR, $sValue, $sName,
 			$bIsSubmit, $sId);
 	}

+	//-------------------------------------------------
+	// Alternative action buttons, mostly used in forms
+	//-------------------------------------------------
+
 	/**
 	 * Make a basis Button component for any purpose
 	 *
-	 * @param string $sLabel
-	 * @param string $sName See Button::$sName
+	 * @param string      $sLabel
+	 * @param string      $sName See Button::$sName
+	 * @param string|null $sValue See Button::$sValue
+	 * @param bool        $bIsSubmit See Button::$sType
 	 * @param string|null $sId
 	 *
 	 * @return \Combodo\iTop\Application\UI\Component\Button\Button
 	 */
-	public static function MakeAlternativeNeutral(string $sLabel, string $sName, ?string $sId = null)
-	{
-		$oButton = new Button($sLabel, $sId);
-		$oButton->SetActionType(Button::ENUM_ACTION_TYPE_ALTERNATIVE)
-			->SetColor(Button::ENUM_COLOR_NEUTRAL)
-			->SetName($sName);
-
-		return $oButton;
+	public static function MakeAlternativeNeutral(
+		string $sLabel,
+		string $sName,
+		string $sValue = null,
+		bool $bIsSubmit = false,
+		?string $sId = null
+	): Button {
+		return static::MakeForAction($sLabel, Button::ENUM_COLOR_NEUTRAL, Button::ENUM_ACTION_TYPE_ALTERNATIVE, $sValue, $sName,
+			$bIsSubmit, $sId);
 	}

 	/**
@@ -172,7 +182,7 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_PRIMARY, Button::ENUM_ACTION_TYPE_ALTERNATIVE, $sValue, $sName,
 			$bIsSubmit, $sId);
 	}
@@ -194,7 +204,7 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_SECONDARY, Button::ENUM_ACTION_TYPE_ALTERNATIVE, $sValue, $sName,
 			$bIsSubmit, $sId);
 	}
@@ -216,7 +226,7 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_VALIDATION, Button::ENUM_ACTION_TYPE_ALTERNATIVE, $sValue, $sName,
 			$bIsSubmit, $sId);
 	}
@@ -238,11 +248,49 @@ class ButtonFactory
 		string $sValue = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		return static::MakeForAction($sLabel, Button::ENUM_COLOR_DESTRUCTIVE, Button::ENUM_ACTION_TYPE_ALTERNATIVE, $sValue, $sName,
 			$bIsSubmit, $sId);
 	}

+	//----------------------------------------------------------------------------------------------
+	// Link buttons, mostly used outside forms, to redirect somewhere whilst keeping a button aspect
+	//----------------------------------------------------------------------------------------------
+
+	/**
+	 * Make a link Button component to open an URL instead of triggering a form action
+	 *
+	 * @param string $sURL
+	 * @param string|null $sLabel
+	 * @param string|null $sIconClass
+	 * @param string|null $sName See Button::$sName
+	 * @param string|null $sTarget
+	 * @param string|null $sId
+	 *
+	 * @return \Combodo\iTop\Application\UI\Component\Button\Button
+	 */
+	public static function MakeLinkNeutral(string $sURL, ?string $sLabel = null, ?string $sIconClass = null, ?string $sName = null, ?string $sTarget = null, ?string $sId = null): Button
+	{
+		$oButton = static::MakeForAction($sLabel, Button::ENUM_COLOR_NEUTRAL, Button::ENUM_ACTION_TYPE_ALTERNATIVE, null, $sName, false, $sId);
+
+		if (!empty($sIconClass)) {
+			$oButton->SetIconClass($sIconClass);
+		}
+
+		if (!empty($sURL)) {
+			if (empty($sTarget)) {
+				$sTarget = "_self";
+			}
+			$oButton->SetOnClickJsCode("window.open('{$sURL}', '{$sTarget}');");
+		}
+
+		return $oButton;
+	}
+
+	//--------
+	// Helpers
+	//--------
+
 	/**
 	 * Internal helper
 	 *
@@ -265,7 +313,7 @@ class ButtonFactory
 		string $sName = null,
 		bool $bIsSubmit = false,
 		?string $sId = null
-	) {
+	): Button {
 		$oButton = new Button($sLabel, $sId);
 		$oButton->SetActionType($sActionType)
 			->SetColor($sColor);
@@ -285,40 +333,4 @@ class ButtonFactory

 		return $oButton;
 	}
-
-
-	/**
-	 * Make a basis Button component for any purpose
-	 *
-	 * @param string $sLabel
-	 * @param string $sName See Button::$sName
-	 * @param string $sIconClass
-	 * @param string $sURL
-	 * @param string $sTarget
-	 * @param string|null $sId
-	 *
-	 * @return \Combodo\iTop\Application\UI\Component\Button\Button
-	 */
-	public static function MakeAlternativeNeutralActionButton(string $sLabel, string $sName, string $sIconClass = '', string $sURL = '', string $sTarget = '', ?string $sId = null): Button
-	{
-		$oButton = new Button($sLabel, $sId);
-		$oButton->SetActionType(Button::ENUM_ACTION_TYPE_ALTERNATIVE)
-			->SetColor(Button::ENUM_COLOR_NEUTRAL)
-			->SetName($sName);
-
-		if (!empty($sIconClass)) {
-			$oButton->SetIconClass($sIconClass);
-		}
-
-		if (!empty($sURL)) {
-			if (empty($sTarget)) {
-				$sJS = "window.location='{$sURL}';";
-			} else {
-				$sJS = "window.open('{$sURL}', '{$sTarget}');";
-			}
-			$oButton->SetOnClickJsCode($sJS);
-		}
-
-		return $oButton;
-	}
 }
--- a/sources/application/UI/Layout/TabContainer/Tab/AjaxTab.php
+++ b/sources/application/UI/Layout/TabContainer/Tab/AjaxTab.php
@@ -36,7 +36,6 @@ class AjaxTab extends Tab
 	// Overloaded constants
 	public const BLOCK_CODE = 'ibo-ajaxtab';
 	public const HTML_TEMPLATE_REL_PATH = 'layouts/tabcontainer/tab/ajaxtab';
-	public const JS_TEMPLATE_REL_PATH = 'layouts/tabcontainer/tab/ajaxtab';

 	protected const TAB_TYPE = TabManager::ENUM_TAB_TYPE_AJAX;

--- a/templates/components/button/layout.html.twig
+++ b/templates/components/button/layout.html.twig
@@ -6,7 +6,9 @@
        {% if oUIBlock.IsDisabled() is same as(true) %} disabled {% endif %}
 >
    {% if oUIBlock.GetIconClass() is not empty %}
-        <span class="ibo-button-icon {{ oUIBlock.GetIconClass() }}"></span>
+        <span class="ibo-button--icon {{ oUIBlock.GetIconClass() }}"></span>
+    {% endif %}
+    {% if oUIBlock.GetLabel() is not empty %}
+        <span class="ibo-button--label">{{ oUIBlock.GetLabel() }}</span>
    {% endif %}
-    {{ oUIBlock.GetLabel() }}
 </button>
--- a/test/VisualTest/Backoffice/RenderAllUiBlocks.php
+++ b/test/VisualTest/Backoffice/RenderAllUiBlocks.php
@@ -1,4 +1,7 @@
 <?php
+
+/** @noinspection PhpUnhandledExceptionInspection */
+
 /*
 * Copyright (C) 2013-2020 Combodo SARL
 *
@@ -26,6 +29,7 @@ use Combodo\iTop\Application\UI\Component\Panel\PanelFactory;
 use Combodo\iTop\Application\UI\Component\QuickCreate\QuickCreateFactory;
 use Combodo\iTop\Application\UI\Layout\PageContent\PageContentFactory;
 use iTopWebPage;
+use utils;

 require_once '../../../approot.inc.php';
 require_once APPROOT.'application/startup.inc.php';
@@ -57,25 +61,27 @@ $oPageContentLayout->AddMainBlock(new Html('<hr/>'));
 // Buttons
 //////////
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeNeutral('Neutral', 'neutral'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeNeutral('Neutral', 'neutral')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeNeutral('Neutral dis.', 'neutral')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForPrimaryAction('Primary'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForPrimaryAction('Primary')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForPrimaryAction('Primary dis.')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForSecondaryAction('Secondary'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForSecondaryAction('Secondary')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForSecondaryAction('Secondary dis.')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForValidationAction('Validation'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForValidationAction('Validation')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForValidationAction('Validation dis.')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForDestructiveAction('Destructive'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForDestructiveAction('Destructive')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForDestructiveAction('Destructive dis.')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeAlternativeNeutral('Alt. neutral', 'alt-neutral'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeAlternativeNeutral('Alt. neutral', 'alt-neutral')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeAlternativeNeutral('Alt. neutral dis.', 'alt-neutral')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativePrimaryAction('Alt. primary'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativePrimaryAction('Alt. primary')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativePrimaryAction('Alt. primary dis.')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeSecondaryAction('Alt. secondary'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeSecondaryAction('Alt. secondary')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeSecondaryAction('Alt. secondary dis.')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeValidationAction('Alt. validation'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeValidationAction('Alt. validation')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeValidationAction('Alt. validation dis.')->SetIsDisabled(true));
 $oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeDestructiveAction('Alt. destructive'));
-$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeDestructiveAction('Alt. destructive')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeDestructiveAction('Alt. destructive dis.')->SetIsDisabled(true));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeLinkNeutral(utils::GetAbsoluteUrlAppRoot(), 'Link neutral'));
+$oPageContentLayout->AddMainBlock(ButtonFactory::MakeForAlternativeDestructiveAction('Alt. destructive dis.')->SetIsDisabled(true));

 $oPageContentLayout->AddMainBlock(new Html('<hr/>'));