Merge pull request 'Move settings button back to the right in repo and org header' (#3462) from Beowulf/forgejo:move-repo-settings-btn-back-to-right into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3462
Reviewed-by: Otto <otto@codeberg.org>
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

templates/org/menu.tmpl

@@ -40,7 +40,7 @@
 			</a>
 			{{end}}
 			{{if .IsOrganizationOwner}}
-			<a class="{{if .PageIsOrgSettings}}active {{end}}item" href="{{.OrgLink}}/settings">
+			<a id="settings-btn" class="{{if .PageIsOrgSettings}}active {{end}}right item" href="{{.OrgLink}}/settings">
 			{{svg "octicon-tools"}} {{ctx.Locale.Tr "repo.settings"}}
 			</a>
 			{{end}}

templates/repo/header.tmpl

@@ -177,18 +177,18 @@
 					{{$highlightSettings := true}}
 					{{if and .SignedUser.EnableRepoUnitHints (not (.Repository.AllUnitsEnabled ctx))}}
 						{{$highlightSettings = false}}
-						<a class="{{if .PageIsRepoSettingsUnits}}active {{end}}item" href="{{.RepoLink}}/settings/units">
+						<a id="settings-btn" class="{{if .PageIsRepoSettingsUnits}}active {{end}}right item" href="{{.RepoLink}}/settings/units">
 							{{svg "octicon-diff-added"}} {{ctx.Locale.Tr "repo.settings.units.add_more"}}
 						</a>
 					{{end}}
-					<a class="{{if and .PageIsRepoSettings (or $highlightSettings (not .PageIsRepoSettingsUnits))}}active {{end}} item" href="{{.RepoLink}}/settings">
+					<a id="settings-btn" class="{{if and .PageIsRepoSettings (or $highlightSettings (not .PageIsRepoSettingsUnits))}}active {{end}}right item" href="{{.RepoLink}}/settings">
 						{{svg "octicon-tools"}} {{ctx.Locale.Tr "repo.settings"}}
 					</a>
 				{{end}}
 			</div>
 		{{else if .Permission.IsAdmin}}
 			<div class="overflow-menu-items">
-				<a class="{{if .PageIsRepoSettings}}active {{end}} item" href="{{.RepoLink}}/settings">
+				<a id="settings-btn" class="{{if .PageIsRepoSettings}}active {{end}}right item" href="{{.RepoLink}}/settings">
 					{{svg "octicon-tools"}} {{ctx.Locale.Tr "repo.settings"}}
 				</a>
 			</div>

tests/e2e/right-settings-button.test.e2e.js

@@ -0,0 +1,114 @@
+// @ts-check
+import {test, expect} from '@playwright/test';
+import {login_user, load_logged_in_context} from './utils_e2e.js';
+
+test.beforeAll(async ({browser}, workerInfo) => {
+  await login_user(browser, workerInfo, 'user2');
+});
+
+test.describe('desktop viewport', () => {
+  test.use({viewport: {width: 1920, height: 300}});
+
+  test('Settings button on right of repo header', async ({browser}, workerInfo) => {
+    const context = await load_logged_in_context(browser, workerInfo, 'user2');
+    const page = await context.newPage();
+
+    await page.goto('/user2/repo1');
+
+    const settingsBtn = page.locator('.overflow-menu-items>#settings-btn');
+    await expect(settingsBtn).toBeVisible();
+    await expect(settingsBtn).toHaveClass(/right/);
+
+    await expect(page.locator('.overflow-menu-button')).toHaveCount(0);
+  });
+
+  test('Settings button on right of org header', async ({browser}, workerInfo) => {
+    const context = await load_logged_in_context(browser, workerInfo, 'user2');
+    const page = await context.newPage();
+
+    await page.goto('/org3');
+
+    const settingsBtn = page.locator('.overflow-menu-items>#settings-btn');
+    await expect(settingsBtn).toBeVisible();
+    await expect(settingsBtn).toHaveClass(/right/);
+
+    await expect(page.locator('.overflow-menu-button')).toHaveCount(0);
+  });
+
+  test('User overview overflow menu should not be influenced', async ({page}) => {
+    await page.goto('/user2');
+
+    await expect(page.locator('.overflow-menu-items>#settings-btn')).toHaveCount(0);
+
+    await expect(page.locator('.overflow-menu-button')).toHaveCount(0);
+  });
+});
+
+test.describe('small viewport', () => {
+  test.use({viewport: {width: 800, height: 300}});
+
+  test('Settings button in overflow menu of repo header', async ({browser}, workerInfo) => {
+    const context = await load_logged_in_context(browser, workerInfo, 'user2');
+    const page = await context.newPage();
+
+    await page.goto('/user2/repo1');
+
+    await expect(page.locator('.overflow-menu-items>#settings-btn')).toHaveCount(0);
+
+    await expect(page.locator('.overflow-menu-button')).toBeVisible();
+
+    await page.click('.overflow-menu-button');
+    await expect(page.locator('.tippy-target>#settings-btn')).toBeVisible();
+
+    // Verify that we have no duplicated items
+    const shownItems = await page.locator('.overflow-menu-items>a').all();
+    expect(shownItems).not.toHaveLength(0);
+    const overflowItems = await page.locator('.tippy-target>a').all();
+    expect(overflowItems).not.toHaveLength(0);
+
+    const items = shownItems.concat(overflowItems);
+    expect(Array.from(new Set(items))).toHaveLength(items.length);
+  });
+
+  test('Settings button in overflow menu of org header', async ({browser}, workerInfo) => {
+    const context = await load_logged_in_context(browser, workerInfo, 'user2');
+    const page = await context.newPage();
+
+    await page.goto('/org3');
+
+    await expect(page.locator('.overflow-menu-items>#settings-btn')).toHaveCount(0);
+
+    await expect(page.locator('.overflow-menu-button')).toBeVisible();
+
+    await page.click('.overflow-menu-button');
+    await expect(page.locator('.tippy-target>#settings-btn')).toBeVisible();
+
+    // Verify that we have no duplicated items
+    const shownItems = await page.locator('.overflow-menu-items>a').all();
+    expect(shownItems).not.toHaveLength(0);
+    const overflowItems = await page.locator('.tippy-target>a').all();
+    expect(overflowItems).not.toHaveLength(0);
+
+    const items = shownItems.concat(overflowItems);
+    expect(Array.from(new Set(items))).toHaveLength(items.length);
+  });
+
+  test('User overview overflow menu should not be influenced', async ({page}) => {
+    await page.goto('/user2');
+
+    await expect(page.locator('.overflow-menu-items>#settings-btn')).toHaveCount(0);
+
+    await expect(page.locator('.overflow-menu-button')).toBeVisible();
+    await page.click('.overflow-menu-button');
+    await expect(page.locator('.tippy-target>#settings-btn')).toHaveCount(0);
+
+    // Verify that we have no duplicated items
+    const shownItems = await page.locator('.overflow-menu-items>a').all();
+    expect(shownItems).not.toHaveLength(0);
+    const overflowItems = await page.locator('.tippy-target>a').all();
+    expect(overflowItems).not.toHaveLength(0);
+
+    const items = shownItems.concat(overflowItems);
+    expect(Array.from(new Set(items))).toHaveLength(items.length);
+  });
+});

web_src/js/webcomponents/overflow-menu.js

@@ -69,13 +69,35 @@ window.customElements.define('overflow-menu', class extends HTMLElement {
     this.tippyItems = [];
     const menuRight = this.offsetLeft + this.offsetWidth;
     const menuItems = this.menuItemsEl.querySelectorAll('.item');
+    const settingItem = this.menuItemsEl.querySelector('#settings-btn');
     for (const item of menuItems) {
       const itemRight = item.offsetLeft + item.offsetWidth;
-      if (menuRight - itemRight < 38) { // roughly the width of .overflow-menu-button
+      // Width of the settings button plus a small value to get the next item to the left if there is directly one
+      // If no setting button is in the menu the default threshold is 38 - roughly the width of .overflow-menu-button
+      const overflowBtnThreshold = 38;
+      const threshold = settingItem?.offsetWidth ?? overflowBtnThreshold;
+      // If we have a settings item on the right-hand side, we must also check if the first,
+      // possibly overflowing item would still fit on the left-hand side of the overflow menu
+      // If not, it must be added to the array (twice). The duplicate is removed with the shift.
+      if (settingItem && !this.tippyItems?.length && item !== settingItem && menuRight - itemRight < overflowBtnThreshold) {
+        this.tippyItems.push(settingItem);
+      }
+      if (menuRight - itemRight < threshold) {
         this.tippyItems.push(item);
       }
     }
 
+    // Special handling for settings button on right. Only done if a setting item is present
+    if (settingItem) {
+      // If less than 2 items overflow, remove all items (only settings "overflowed" - because it's on the right side)
+      if (this.tippyItems?.length < 2) {
+        this.tippyItems = [];
+      } else {
+        // Remove the first item of the list, because we have always one item more in the array due to the big threshold above
+        this.tippyItems.shift();
+      }
+    }
+
     // if there are no overflown items, remove any previously created button
     if (!this.tippyItems?.length) {
       const btn = this.querySelector('.overflow-menu-button');