mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
feat(ui): improve visibility of counters inside of switch items (#11472)
Apply the feature from https://codeberg.org/forgejo/forgejo/pulls/2935 on element from https://codeberg.org/forgejo/forgejo/pulls/6459 (which was applied to notifications page in https://codeberg.org/forgejo/forgejo/pulls/6542). A few small semi-related refactors. One of them (nested CSS commit) actually revealed a hole in testing: there are no test cases for hover in `evaluateSwitchItem`. I would like to address this but this PR already conflicts with https://codeberg.org/forgejo/forgejo/pulls/11341, so I won't do that until either is merged to save on rebase work. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11472 Reviewed-by: Gusted <gusted@noreply.codeberg.org> Reviewed-by: Beowulf <beowulf@beocode.eu> Co-authored-by: 0ko <0ko@noreply.codeberg.org> Co-committed-by: 0ko <0ko@noreply.codeberg.org>
This commit is contained in:
0ko
Gusted
parent
d442f83a09
commit
d665904a22
3 changed files with 111 additions and 51 deletions
|
|
@ -5,7 +5,7 @@
|
|||
<div class="switch">
|
||||
<a class="{{if eq .Status 1}}active {{end}}item" href="{{AppSubUrl}}/notifications?q=unread">
|
||||
{{ctx.Locale.Tr "notification.unread"}}
|
||||
<div class="notifications-unread-count ui label {{if not $notificationUnreadCount}}tw-hidden{{end}}">{{$notificationUnreadCount}}</div>
|
||||
<div class="notifications-unread-count ui label{{if not $notificationUnreadCount}} tw-hidden{{end}}">{{$notificationUnreadCount}}</div>
|
||||
</a>
|
||||
<a class="{{if eq .Status 2}}active {{end}}item" href="{{AppSubUrl}}/notifications?q=read">
|
||||
{{ctx.Locale.Tr "notification.read"}}
|
||||
|
|
|
|||
|
|
@ -7,15 +7,25 @@
|
|||
// web_src/css/themes
|
||||
// @watch end
|
||||
|
||||
import {expect} from '@playwright/test';
|
||||
import {test, login_user} from './utils_e2e.ts';
|
||||
import {expect, type Page} from '@playwright/test';
|
||||
import {test} from './utils_e2e.ts';
|
||||
|
||||
test.describe('Switch CSS properties', () => {
|
||||
const noBg = 'rgba(0, 0, 0, 0)';
|
||||
const activeBg = 'rgb(226, 226, 229)';
|
||||
const hoverBg = 'rgba(228, 228, 228, 0.667)';
|
||||
|
||||
async function evaluateSwitchItem(page, selector, isActive, marginLeft, marginRight, paddingLeft, paddingRight, itemHeight) {
|
||||
const normalMargin = '0px';
|
||||
const normalPadding = '15.75px';
|
||||
|
||||
const specialLeftMargin = '-4px';
|
||||
const specialPadding = '19.75px';
|
||||
|
||||
async function evaluateSwitchItem(page: Page, selector: string, hover, isActive: boolean, marginLeft, marginRight, paddingLeft, paddingRight: string, itemHeight: number) {
|
||||
const item = page.locator(selector);
|
||||
|
||||
if (hover) await item.hover();
|
||||
|
||||
const cs = await item.evaluate((el) => {
|
||||
// In Firefox getComputedStyle is undefined if returned from evaluate
|
||||
const s = getComputedStyle(el);
|
||||
|
|
@ -34,21 +44,29 @@ test.describe('Switch CSS properties', () => {
|
|||
|
||||
if (isActive) {
|
||||
await expect(item).toHaveClass(/active/);
|
||||
|
||||
// Active item has active background color regardless of `hover`
|
||||
expect(cs.backgroundColor).toBe(activeBg);
|
||||
} else {
|
||||
await expect(item).not.toHaveClass(/active/);
|
||||
expect(cs.backgroundColor).toBe(noBg);
|
||||
|
||||
// When hovering, `getComputedStyle` returns random `backgroundColor` values
|
||||
// because of transition. `toHaveCSS` is reliable
|
||||
if (hover) {
|
||||
// Verify that inactive item changes it's background color on hover
|
||||
await expect(item).toHaveCSS('background-color', hoverBg);
|
||||
} else {
|
||||
// Verify that inactive item doesn't have a background color
|
||||
await expect(item).toHaveCSS('background-color', noBg);
|
||||
}
|
||||
}
|
||||
|
||||
expect((await item.boundingBox()).height).toBeCloseTo(itemHeight);
|
||||
expect((await item.boundingBox()).height).toBeCloseTo(itemHeight, 1);
|
||||
|
||||
// Reset hover
|
||||
if (hover) await page.locator('#navbar-logo').hover();
|
||||
}
|
||||
|
||||
const normalMargin = '0px';
|
||||
const normalPadding = '15.75px';
|
||||
|
||||
const specialLeftMargin = '-4px';
|
||||
const specialPadding = '19.75px';
|
||||
|
||||
// Subtest for areas that can be evaluated without JS
|
||||
test('No JS', async ({browser}) => {
|
||||
const context = await browser.newContext({javaScriptEnabled: false});
|
||||
|
|
@ -60,9 +78,9 @@ test.describe('Switch CSS properties', () => {
|
|||
|
||||
await expect(async () => {
|
||||
await Promise.all([
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(1)', true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(2)', false, specialLeftMargin, normalMargin, specialPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(3)', false, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(1)', false, true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(2)', false, false, specialLeftMargin, normalMargin, specialPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(3)', false, false, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
]);
|
||||
}).toPass();
|
||||
|
||||
|
|
@ -70,9 +88,9 @@ test.describe('Switch CSS properties', () => {
|
|||
|
||||
await expect(async () => {
|
||||
await Promise.all([
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(1)', false, normalMargin, specialLeftMargin, normalPadding, specialPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(2)', true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(3)', false, specialLeftMargin, normalMargin, specialPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(1)', false, false, normalMargin, specialLeftMargin, normalPadding, specialPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(2)', false, true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(3)', false, false, specialLeftMargin, normalMargin, specialPadding, normalPadding, itemHeight),
|
||||
]);
|
||||
}).toPass();
|
||||
|
||||
|
|
@ -80,32 +98,68 @@ test.describe('Switch CSS properties', () => {
|
|||
|
||||
await expect(async () => {
|
||||
await Promise.all([
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(1)', false, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(2)', false, normalMargin, specialLeftMargin, normalPadding, specialPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(3)', true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(1)', false, false, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(2)', false, false, normalMargin, specialLeftMargin, normalPadding, specialPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(3)', false, true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
]);
|
||||
}).toPass();
|
||||
|
||||
// Check colors on hover synchronously - can only hover one item at a time
|
||||
await expect(async () => {
|
||||
await evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(1)', true, false, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight);
|
||||
await evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(2)', true, false, normalMargin, specialLeftMargin, normalPadding, specialPadding, itemHeight);
|
||||
await evaluateSwitchItem(page, '#issue-filters .switch > .item:nth-child(3)', true, true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight);
|
||||
}).toPass();
|
||||
});
|
||||
|
||||
// Subtest for areas that can't be reached without JS
|
||||
test('With JS', async ({browser}, workerInfo) => {
|
||||
const context = await login_user(browser, workerInfo, 'user2');
|
||||
const page = await context.newPage();
|
||||
test.describe('With JS', () => {
|
||||
test.use({user: 'user2'});
|
||||
|
||||
// Go to files tab of a reviewable pull request
|
||||
await page.goto('/user2/repo1/pulls/5/files');
|
||||
test('PR review box', async ({page}) => {
|
||||
// Go to files tab of a reviewable pull request
|
||||
await page.goto('/user2/repo1/pulls/5/files');
|
||||
|
||||
// Open review box
|
||||
await page.locator('#review-box .js-btn-review').click();
|
||||
// Open review box
|
||||
await page.locator('#review-box .js-btn-review').click();
|
||||
|
||||
// Markdown editor has a special rule for a shorter switch
|
||||
const itemHeight = 28;
|
||||
// Markdown editor has a special rule for a shorter switch
|
||||
const itemHeight = 28;
|
||||
|
||||
await expect(async () => {
|
||||
await Promise.all([
|
||||
evaluateSwitchItem(page, '.review-box-panel .switch > .item:nth-child(1)', true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '.review-box-panel .switch > .item:nth-child(2)', false, specialLeftMargin, normalMargin, specialPadding, normalPadding, itemHeight),
|
||||
]);
|
||||
}).toPass();
|
||||
await expect(async () => {
|
||||
await Promise.all([
|
||||
evaluateSwitchItem(page, '.review-box-panel .switch > .item:nth-child(1)', false, true, normalMargin, normalMargin, normalPadding, normalPadding, itemHeight),
|
||||
evaluateSwitchItem(page, '.review-box-panel .switch > .item:nth-child(2)', false, false, specialLeftMargin, normalMargin, specialPadding, normalPadding, itemHeight),
|
||||
]);
|
||||
}).toPass();
|
||||
});
|
||||
|
||||
test('Notifications page', async ({page}) => {
|
||||
// Test counter contrast boost in active and :hover items
|
||||
const labelBgNormal = 'rgba(202, 202, 202, 0.482)';
|
||||
const labelBgContrast = 'rgb(202, 202, 202)';
|
||||
const counter = page.locator('a.item[href="/notifications?q=unread"] > .ui.label');
|
||||
|
||||
// On Unread tab (item with counter is active)
|
||||
await page.goto('/notifications?q=unread');
|
||||
|
||||
// * not hovering => boosted because .active
|
||||
expect(await counter.evaluate((el) => getComputedStyle(el).backgroundColor)).toBe(labelBgContrast);
|
||||
|
||||
// * hoveing => boosted
|
||||
await page.locator('a.item[href="/notifications?q=unread"]').hover();
|
||||
expect(await counter.evaluate((el) => getComputedStyle(el).backgroundColor)).toBe(labelBgContrast);
|
||||
|
||||
// On Read tab (item with counter is inactive)
|
||||
await page.goto('/notifications?q=read');
|
||||
|
||||
// * not hovering => normal
|
||||
await page.locator('#navbar-logo').hover();
|
||||
expect(await counter.evaluate((el) => getComputedStyle(el).backgroundColor)).toBe(labelBgNormal);
|
||||
|
||||
// * hoveing => boosted
|
||||
await page.locator('a.item[href="/notifications?q=unread"]').hover();
|
||||
expect(await counter.evaluate((el) => getComputedStyle(el).backgroundColor)).toBe(labelBgContrast);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -34,14 +34,26 @@
|
|||
border-radius: var(--border-radius);
|
||||
text-wrap: nowrap;
|
||||
transition: background-color 0.1s ease;
|
||||
|
||||
&:hover {
|
||||
background: var(--color-hover);
|
||||
}
|
||||
|
||||
&.active {
|
||||
z-index: 2;
|
||||
padding-left: var(--switch-padding-inline);
|
||||
background: var(--color-active);
|
||||
outline: 1px solid var(--color-input-border);
|
||||
}
|
||||
|
||||
/* Prevent default browser styling */
|
||||
&button {
|
||||
background: transparent;
|
||||
}
|
||||
}
|
||||
|
||||
.switch > .item:hover {
|
||||
background: var(--color-hover);
|
||||
}
|
||||
|
||||
/* Item that has to crawl under it's active neighbor, so when it is hovered,
|
||||
there are no ugly unpainted v/^ shapes between them */
|
||||
/* Corner rounding aid: item that has to crawl under it's active neighbor,
|
||||
so when it is hovered, there are no ugly unpainted v/^ shapes between them */
|
||||
.switch > .item:has(+ .active.item) { /* Active neighbor is next item */
|
||||
margin-right: calc(-1 * var(--border-radius));
|
||||
padding-right: calc(var(--switch-padding-inline) + var(--border-radius));
|
||||
|
|
@ -51,13 +63,7 @@ there are no ugly unpainted v/^ shapes between them */
|
|||
padding-left: calc(var(--switch-padding-inline) + var(--border-radius));
|
||||
}
|
||||
|
||||
.switch > .active.item {
|
||||
z-index: 2;
|
||||
padding-left: var(--switch-padding-inline);
|
||||
background: var(--color-active);
|
||||
outline: 1px solid var(--color-input-border);
|
||||
}
|
||||
|
||||
.switch > button.item {
|
||||
background: transparent;
|
||||
/* Make counters embedded into items more visible on brighter backgrounds */
|
||||
.switch > .item:is(.active, :hover) > .ui.label {
|
||||
background: var(--color-label-bg-alt, var(--color-label-bg));
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue