Page MenuHomePhabricator

Cosmos skin: Refactor CosmosToolbar and CosmosNavigation
Open, MediumPublic

Description

Redundant checks

The following methods getCode() and getMenuLines() (defined in both classes) have redundant checks per variable scope.

The $menu variable is a local variable within the method (not a parameter), and will always be considered null first, and then assigned $this->getMenu($this->getMenuLines()). This basically goes the same way with the local $lines variable defined in getMenuLines().

Before

	public function getCode() {
		if (empty($menu)) {
			$menu = $this->getMenu($this->getMenuLines());
		}
		return $menu;
	}

	public function getMenuLines() {
		if (empty($lines)) {
			$lines = self::getMessageAsArray('Cosmos-navigation');
		}
		return $lines;
	}

After

	public function getCode() {
		return $this->getMenu($this->getMenuLines());
	}

	public function getMenuLines() {
		return self::getMessageAsArray('Cosmos-navigation');
	}

Duplicated classes

These classes *mostly* do the same thing. This should be refactored. The differences:

  • CosmsoNavigation defines an extra method at CosmosNavigation::getSubMenu()
  • CosmosToolbar uses Cosmos-toolbar as an i18n message, while CosmosNavigation uses Cosmos-navigation
  • CosmosToolbar registers the getCosmosToolbar hook, while CosmosNavigation registers getCosmosNavigation as a hook
  • CosmosNavigation generates a bit more HTML
diff --git a/includes/CosmosToolbar.php b/includes/CosmosNavigation.php
index d1ac9d9..8e4572f 100644
--- a/includes/CosmosToolbar.php
+++ b/includes/CosmosNavigation.php
@@ -1,6 +1,6 @@
 <?php
 /**
- * CosmosToolbar class
+ * CosmosNavigation class
  *
  * @package MediaWiki
  * @subpackage Skins
@@ -14,7 +14,9 @@ if (!defined('MEDIAWIKI')) {
        die(-1);
 }
 
-class CosmosToolbar {
+use Cosmos\Icon;
+
+class CosmosNavigation {
 
        /**
         * Parse one line from MediaWiki message to array with indexes 'text' and 'href'
@@ -116,32 +118,84 @@ class CosmosToolbar {
 
        public function getMenuLines() {
                if (empty($lines)) {
-                       $lines = self::getMessageAsArray('Cosmos-toolbar');
+                       $lines = self::getMessageAsArray('Cosmos-navigation');
                }
 
                return $lines;
        }
 
+       public function getSubMenu($nodes, $children, $bolded = false) {
+               $menu = '';
+               $val = 0;
+               foreach ($children as $key => $val) {
+                       $link_html = htmlspecialchars($nodes[$val]['text']);
+                       if (!empty($nodes[$val]['children'])) {
+                               $link_html .= Icon::getIcon('level-2-dropdown')->makeSvg(12, 12, ['id' => 'wds-icons-menu-control-tiny', 'class' => 'wds-icon wds-icon-tiny wds-dropdown-chevron']);
+                       }
+
+                       $menu_item = Html::rawElement('a', array(
+                               'href' => !empty($nodes[$val]['href']) ? $nodes[$val]['href'] : '#',
+                               'class' => (!empty($nodes[$val]['children']) ? 'wds-dropdown-level-2__toggle' : null),
+                               'rel' => $nodes[$val]['internal'] ? null : 'nofollow'
+                       ) , $link_html) . "\n";
+                       if (!empty($nodes[$val]['children'])) {
+                               $menu_item .= '<div class="wds-is-not-scrollable wds-dropdown-level-2__content" id="p-'. Sanitizer::escapeIdForAttribute($nodes[$val]['text']) . '" aria-labelledby="p-' . Sanitizer::escapeIdForAttribute($nodes[$val]['text']) . '-label">';  
+                               $menu_item .= $this->getSubMenu($nodes, $nodes[$val]['children']);
+                               $menu_item .= '</div>';
+                       }
+                       $menu .= Html::rawElement('li',  array(
+                               'id' => Sanitizer::escapeIdForAttribute( 'n-' . strtr( $nodes[$val]['text'], ' ', '-' ) ),
+                               'class' => (!empty($nodes[$val]['children']) ? ($key > count($nodes[$val]['children']) - 1 ? 'wds-is-sticked-to-parent ' : '') . 'wds-dropdown-level-2' : false)
+                       ), $menu_item);
+               }
+               
+               $menu = Html::rawElement('div', [], '<ul class="wds-list wds-is-linked' . ($bolded === true ? ' wds-has-bolded-items' : '') . '">' . $menu . '</ul>');
+
+               return $menu;
+       }
+
        public function getMenu($lines) {
                $menu = '';
                $nodes = $this->parse($lines);
 
                if (count($nodes) > 0) {
 
-                       Hooks::run('getCosmosToolbar', array(&$nodes
-                       ));
+                       Hooks::run('getCosmosNavigation', array(&$nodes));
 
                        $mainMenu = array();
+
                        foreach ($nodes[0]['children'] as $key => $val) {
-                               $menu .= '<li id="' . Sanitizer::escapeIdForAttribute( 't-' . strtolower(strtr( $nodes[$val]['text'], ' ', '-' ) ) ) . '">';
-                               $menu .= '<a href="' . (!empty($nodes[$val]['href']) ? htmlspecialchars($nodes[$val]['href']) : '#') . '"';
+                               if (isset($nodes[$val]['children'])) {
+                                       $mainMenu[$val] = $nodes[$val]['children'];
+                               }
+                               $menu .= '<li class="wds-tabs__tab">';
+                               if (!empty($nodes[$val]['children'])) {
+                                       $menu .= '<div class="wds-dropdown" id="p-' . Sanitizer::escapeIdForAttribute($nodes[$val]['text']) . '" aria-labelledby="p-' . Sanitizer::escapeIdForAttribute($nodes[$val]['text']) . '-label">';
+                               }
+                               $menu .= '<div class="wds-tabs__tab-label ';
+                               if (!empty($nodes[$val]['children'])) {
+                                       $menu .= ' wds-dropdown__toggle';
+                               }
+                               $menu .= '" id="p-' . Sanitizer::escapeIdForAttribute($nodes[$val]['text']) . '-label">';
+                               $menu .= '<a href="' . (!empty($nodes[$val]['href']) && $nodes[$val]['text'] !== 'Navigation' && $nodes[$val]['text'] !== wfMessage('cosmos-explore')->text() ? htmlspecialchars($nodes[$val]['href']) : '#') . '"';
                                if (!isset($nodes[$val]['internal']) || !$nodes[$val]['internal']) $menu .= ' rel="nofollow"';
-                               $menu .= '><span>' . htmlspecialchars($nodes[$val]['text']) . '</span>';
-                               $menu .= '</a>';
+                               $menu .= '>' . ($nodes[$val]['text'] === wfMessage('cosmos-explore')->text() ? Icon::getIcon('explore')->makeSvg(91, 91, ['id' => 'cosmos-icons-explore', 'class' => 'wds-icon']) : '') . '<span ' . ($nodes[$val]['text'] === wfMessage('cosmos-explore')->text() ? 'style="padding-top: 2px;"' : '') . '>' . htmlspecialchars($nodes[$val]['text']) . '</span>';
+                               if (!empty($nodes[$val]['children'])) {
+                                       $menu .= Icon::getIcon('dropdown')->makeSvg(14, 14, ['id' => 'wds-icons-dropdown-tiny', 'class' => 'wds-icon wds-icon-tiny wds-dropdown__toggle-chevron']);
+                               }
+                               $menu .= '</a></div>';
+                               if (!empty($nodes[$val]['children'])) {
+                                       $menu .= '<div class="wds-is-not-scrollable wds-dropdown__content">';
+                                       $menu .= $this->getSubMenu($nodes, $nodes[$val]['children'], true);
+                                       $menu .= '</div></div>';
+                               }
 
                        }
                        $menu .= '</li>';
-                       
+
+                       $menu = Html::rawElement('li', array(
+                               'class' => 'wds-tabs__tab'
+                       ) , $menu);
                        $menu = preg_replace('/<!--b-->(.*)<!--e-->/U', '', $menu);
 
                        $menuHash = hash('md5', serialize($nodes));
@@ -159,6 +213,8 @@ class CosmosToolbar {
 
                        $memc = ObjectCache::getLocalClusterInstance();
+                                       $menu .= '<div class="wds-is-not-scrollable wds-dropdown__content">';
+                                       $menu .= $this->getSubMenu($nodes, $nodes[$val]['children'], true);
+                                       $menu .= '</div></div>';
+                               }
 
                        }
                        $menu .= '</li>';
-                       
+
+                       $menu = Html::rawElement('li', array(
+                               'class' => 'wds-tabs__tab'
+                       ) , $menu);
                        $menu = preg_replace('/<!--b-->(.*)<!--e-->/U', '', $menu);
 
                        $menuHash = hash('md5', serialize($nodes));
@@ -159,6 +213,8 @@ class CosmosToolbar {
 
                        $memc = ObjectCache::getLocalClusterInstance();

Event Timeline

SamanthaNguyen triaged this task as Medium priority.Oct 10 2020, 5:25 PM

This should be looked at once PHPCS styling issues are done (T265108) and re-namespacing is done (T265107).

Change 633368 had a related patch set uploaded (by SamanthaNguyen; owner: SamanthaNguyen):
[mediawiki/skins/Cosmos@master] Some basic code cleanup between CosmosNavigation & CosmosToolbar

https://gerrit.wikimedia.org/r/633368

While we're at this, I'm currently wondering if CosmosNavigation::parseItem() and CosmosToolbar::parseItem() can be removed safely.
T hey don't seem to be used within the Cosmos source code according to Codesearch https://codesearch.wmcloud.org/skins/?q=parseItem&i=nope&files=&repos=Skin:Cosmos

Change 633368 merged by jenkins-bot:
[mediawiki/skins/Cosmos@master] Some basic code cleanup between CosmosNavigation & CosmosToolbar

https://gerrit.wikimedia.org/r/633368

Change 634352 had a related patch set uploaded (by SamanthaNguyen; owner: SamanthaNguyen):
[mediawiki/skins/Cosmos@master] Inject a MessageLocalizer into CosmosNavigation and CosmosToolbar

https://gerrit.wikimedia.org/r/634352

Change 634352 merged by jenkins-bot:
[mediawiki/skins/Cosmos@master] Replace most calls to the global wfMessage() function

https://gerrit.wikimedia.org/r/634352

Change 634264 had a related patch set uploaded (by Universal Omega; owner: Universal Omega):
[mediawiki/skins/Cosmos@master] Remove unused ParseItem from CosmosNavigation and CosmosToolbar classes

https://gerrit.wikimedia.org/r/634264

Change 634264 merged by jenkins-bot:
[mediawiki/skins/Cosmos@master] Remove unused parseItem() function from the CosmosNavigation and CosmosToolbar classes

https://gerrit.wikimedia.org/r/634264