Conversation
|
Visit the preview URL for this PR (updated for commit ab77ddb): https://koobiq-next--prs-2064-gsbpp4pz.web.app (expires Mon, 21 Sep 2026 07:11:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c |
🚨 E2E tests failedReview the report for details. 💡 Comment |
lskramarov
left a comment
lskramarov
left a comment
There was a problem hiding this comment.
I wrote these comments at 0b91663 and re-checked them at 56d85e5. That commit updates the 01-light/01-dark baselines, which fixes the stale-baseline problem I had found, and merging main doesn't change the code involved.
How I checked: I served the e2e app at the merge base b8e2a4889 and at this branch, then rendered the same fixtures in Chromium, plus some edge cases, and compared layout, screenshots and tooltips. I also ran the new unit tests against both commits. The pixel figures in the inline comments come from those renders.
Summary
- The targeted case, a short name next to a long multi-select value, is fixed.
- Capping the whole pipe takes 24–54px from every capped label. Two short labels are now cut off with no ellipsis, and ordinary datetime ranges truncate with no tooltip.
- The cap also limits the input pipe and custom pipes with a wrapped trigger.
kbqPipeMinWidthon date/datetime values goes stale in bars without[filter].- The new unit tests can't fail on the width behaviour, and the e2e tests check the inline style rather than whether the short label is truncated.
The description says kbqPipeMinWidth is now applied to Select and TreeSelect, but those templates already had it; the date and datetime template changes aren't mentioned.
Generated with Claude Code
| [class]="{ 'kbq-active': select.panelOpen }" | ||
| > | ||
| <span #kbqTitleText class="kbq-pipe__name">{{ data.name }}</span> | ||
| <span #kbqTitleText kbqPipeMinWidth class="kbq-pipe__name">{{ data.name }}</span> |
There was a problem hiding this comment.
Two labels under 20 characters are now hard-clipped, with no ellipsis and no tooltip.
kbqPipeMinWidth pins every span shorter than 20 characters at min-width: fit-content. Meanwhile the label area of a capped pipe shrank from 320px to 266px with a clear button (about 296px without one), because the cap now includes the padding and the button.
I rendered a cleanable pipe Ответственный отдел (19) + Отдел безопасности (18) in Chromium, merge base vs this branch, same fixture:
- merge base: 295px of text in a 320px label, fully visible;
- this branch:
.kbq-button-textis 266px wide for 295px of content, so the value is cut off mid-letter ("Отдел безопасн"). Neither span overflows its own box, so no ellipsis is drawn, andkbq-titleshows no tooltip because it compares each span against the wrapper.
Same result for the select, multi-select, text, readonly and multi-tree-select pipes. This PR also adds the pin to the multi-select, date and datetime spans, which previously shrank with an ellipsis.
There was a problem hiding this comment.
Исправил в 81dce2f: убрал порог по количеству символов и kbqPipeMinWidth. Название и значение теперь размещаются в отдельных shrinkable max-content grid-треках, поэтому короткая часть сохраняет ширину, а две более длинные части обрезаются каждая со своим ellipsis. Добавил Playwright-кейс Ответственный отдел + Отдел безопасности: обе части действительно переполняют свои треки, а тултип показывает оба полных значения.
| <span #kbqTitleText class="kbq-pipe__name">{{ data.name }}</span> | ||
| <span #kbqTitleText class="kbq-pipe__value" [class.kbq-pipe__value_empty]="!formattedValue"> | ||
| <span #kbqTitleText kbqPipeMinWidth class="kbq-pipe__name">{{ data.name }}</span> | ||
| <span #kbqTitleText kbqPipeMinWidth class="kbq-pipe__value" [class.kbq-pipe__value_empty]="!formattedValue"> |
There was a problem hiding this comment.
kbqPipeMinWidth keeps a stale fit-content on date/datetime values when no [filter] is bound.
The directive re-measures only after the first render and when filterBar.filter() changes. KbqFilterBar updates filter on onChangePipe only when a filter is bound (if (current) in filter-bar.ts). Without [filter], as in the filter-bar-date-limits docs example, the empty value is measured once (fit-content) and never again.
To reproduce: take a cleanable date pipe Дата обнаружения and pick За последние три календарных месяца. The value keeps min-width: fit-content, and .kbq-button-text is 266px wide for 407px of content, so the value is clipped with no ellipsis. The same pipe in a bar with [(filter)] re-measures to unset and gets an ellipsis. At the merge base both bars show an ellipsis. pipe-datetime.html has the same span. The single-value span in pipe-multi-select.html should go stale the same way when new pipeTemplates relabel an option while it stays the only one selected.
There was a problem hiding this comment.
Убрал kbqPipeMinWidth из date/datetime и остальных встроенных pipe. Раскладка теперь определяется CSS и больше не зависит от обновления [filter], поэтому устаревшего fit-content не остаётся. Поведение обрезанного datetime проверяется в Playwright на обычном E2eFilterBarStates.
| <span #kbqTitleText class="kbq-pipe__name">{{ data.name }}</span> | ||
| <span #kbqTitleText class="kbq-pipe__value" [class.kbq-pipe__value_empty]="!formattedValue"> | ||
| <span #kbqTitleText kbqPipeMinWidth class="kbq-pipe__name">{{ data.name }}</span> | ||
| <span #kbqTitleText kbqPipeMinWidth class="kbq-pipe__value" [class.kbq-pipe__value_empty]="!formattedValue"> |
There was a problem hiding this comment.
Ordinary datetime ranges now lose their end, and hovering doesn't show it.
With the cap on the whole pipe, 12 дек 2025, 10:10 — 9 дек 2025, 10:10 (255px) gets 237px on a plain pipe and 198px on a cleanable one. At the merge base it got 255 and 253 of 255 (pipes #30/#31 in E2eFilterBarStates). Hovering either pipe opens no kbq-title tooltip: isElementOverflown compares each #kbqTitleText span's scrollWidth (255) with the wrapper width (296/266). A value truncated because of its sibling never counts as overflown, so users can only read the end of the range by opening the pipe.
There was a problem hiding this comment.
Добавил локальную для Filter Bar директиву KbqPipeTitle: она сравнивает scrollWidth и clientWidth каждого текстового grid-трека, а не текста со всей шириной триггера. Playwright-тест воспроизводит именно этот случай — значение уже шире своего трека, но ещё уже кнопки — и проверяет, что по hover появляется полный datetime-диапазон.
| display: flex; | ||
| flex-direction: row; | ||
|
|
||
| max-width: var(--kbq-filter-bar-pipe-max-width, 320px); |
There was a problem hiding this comment.
The host cap now also clamps the input pipe.
kbq-pipe-input has the kbq-pipe host class and loads base-pipe.scss, so this max-width overrides the fixed width set in pipe-input.scss. The token comment calls that value "a width rather than a max-width". In a render, a bar with --kbq-filter-bar-pipe-input-width: 400px gets a 400px input at the merge base and a 320px one on this branch. Lowering --kbq-filter-bar-pipe-max-width below 240px now also shrinks the default input. Adding max-width: none in pipe-input.scss, or keeping the cap off the input pipe, would preserve the documented token.
There was a problem hiding this comment.
Добавил max-width: none для .kbq-pipe.kbq-pipe__input, поэтому общий cap pipe больше не переопределяет --kbq-filter-bar-pipe-input-width. Сборка компонентов и целевые тесты проходят; отдельного browser assertion на значение 400px в этом коммите нет.
| max-height: 32px; | ||
| min-width: 0; | ||
|
|
||
| & > .kbq-button, |
There was a problem hiding this comment.
A custom pipe whose trigger isn't a direct child stops truncating.
The old cap was a fixed-length descendant rule, so it limited a kbq-button at any depth. Now only direct .kbq-button / .kbq-select / .kbq-tree-select children get min-width: 0, and the remaining max-width: 100% doesn't limit their min-content width. I wrapped the trigger of E2eFilterBarPipeTruncation in a <div>, as a consumer pipe built on the public KbqBasePipe may do. At the merge base the pipe stays 344px and the value gets an ellipsis. On this branch the host is 320px and the button overflows it by 208px with nothing truncated. In the same render, replacing the class list with & > :not(.kbq-pipe-button) { min-width: 0; } restored truncation.
There was a problem hiding this comment.
Учёл вложенный custom trigger: непосредственный wrapper теперь получает min-width: 0 через & > :not(.kbq-pipe-button), а button/select/tree-select ограничиваются независимо от глубины. Grid-правило для .kbq-button-text также работает по descendant-селектору. Целевой Playwright-регресс обрезания проходит, но отдельный fixture с дополнительным wrapper в коммит не добавлен.
| name, | ||
| type, | ||
| value, | ||
| cleanable: hasLongText, |
There was a problem hiding this comment.
The dev matrix never shows the combination this change breaks.
cleanable comes from the text length (hasLongText). As a result, every "Короткое название + короткое значение" case renders without a clear button, while every other filled case has one. Each row varies two things at once, and short labels next to a clear button never appear, although that is the layout that now clips (see pipe-multi-select.html). Тип/Файл would be too short to show it anyway. A pair such as Ответственный отдел + Отдел безопасности with cleanable: true would show it. Also, createTextPipeTruncationCases duplicates createPipeTruncationCase for the Text group.
There was a problem hiding this comment.
Добавил в dev-матрицу cleanable-кейс Ответственный отдел + Отдел безопасности и отвязал cleanable от длины текста. Также убрал дублирующий генератор Text-кейсов: теперь все группы строятся через один helper. Этот же medium-сценарий покрыт Playwright-проверкой обрезания и тултипа.
| > | ||
| <span #kbqTitleText class="kbq-pipe__name">{{ data.name }}</span> | ||
| <span #kbqTitleText class="kbq-pipe__value" [class.kbq-pipe__value_empty]="!formattedValue"> | ||
| <span #kbqTitleText kbqPipeMinWidth class="kbq-pipe__name">{{ data.name }}</span> |
There was a problem hiding this comment.
Which part keeps its width still depends on each template opting in, and on a character count.
Before this PR, 4 of the 8 built-in templates lacked kbqPipeMinWidth: date, datetime and multi-select since they were added, plus the multi-tree-select value. Custom pipes have to import it too. innerText.length < 20 knows neither pixel widths nor the sibling's width, and it only re-measures on filter() changes. The clipping and staleness described in the other comments come from this.
A layout rule in base-pipe.scss would need neither. In a quick render, .kbq-button-text { display: grid; grid-auto-flow: column; grid-auto-columns: minmax(0, max-content); } kept Тип whole beside the long value, and truncated both halves of the 19+18 pair with an ellipsis instead of clipping. It splits space equally rather than using the 20-character rule, so a short part stays whole only while it fits in half the space. That makes it a design trade-off rather than a drop-in replacement, but it would remove both the measuring and the opt-in.
There was a problem hiding this comment.
Перешёл на CSS-подход: .kbq-button-text использует grid-auto-columns: minmax(0, max-content). kbqPipeMinWidth удалён из всех встроенных шаблонов и custom-pipe-примера, поэтому больше нет opt-in по каждому template, порога в 20 символов и зависимости от обновления filter. Playwright проверяет обе комбинации short/long и случай, когда обе части обрезаны.
| min-width: 0; | ||
| max-width: 100%; | ||
| } | ||
|
|
||
| & .kbq-select .kbq-select__trigger { | ||
| height: unset; | ||
| } | ||
|
|
||
| .kbq-button-wrapper { | ||
| max-width: var(--kbq-filter-bar-pipe-max-width, 320px); | ||
| max-width: 100%; | ||
| min-width: 0; | ||
| } |
There was a problem hiding this comment.
Three of these declarations change nothing.
.kbq-button already has max-width: 100% (%kbq-button-clamped-to-container), .kbq-select / .kbq-tree-select are width: 100% (%kbq-select-base), and .kbq-button-wrapper already gets min-width: 0 from _button-base.scss. I removed the max-width: 100% here and the whole .kbq-button-wrapper block and confirmed in the CSSOM that the rules were gone. E2eFilterBarStates, E2eFilterBarPipeTruncation, the new fixture and several edge cases all rendered pixel-identical. Only min-width: 0 on the direct children has an effect.
| min-width: 0; | |
| max-width: 100%; | |
| } | |
| & .kbq-select .kbq-select__trigger { | |
| height: unset; | |
| } | |
| .kbq-button-wrapper { | |
| max-width: var(--kbq-filter-bar-pipe-max-width, 320px); | |
| max-width: 100%; | |
| min-width: 0; | |
| } | |
| min-width: 0; | |
| } | |
| & .kbq-select .kbq-select__trigger { | |
| height: unset; | |
| } |
There was a problem hiding this comment.
Удалил неработающие max-width: 100% и блок .kbq-button-wrapper. Оставил только влияющие на shrink правила min-width: 0, расширив их на wrapper и вложенные triggers для custom pipe. После упрощения прошли browser-сценарии обрезания, unit-тесты и сборка компонентов.
| const getComponent = (page: Page) => page.getByTestId('e2eFilterBarMultiSelectPipeTruncation'); | ||
|
|
||
| /** Width the text would need if nothing clipped it, versus the width it actually got. */ | ||
| const getWidths = (locator: Locator) => |
There was a problem hiding this comment.
This helper and the two tests duplicate code already in this file.
This getWidths is byte-identical to the one at line 23, and the two new tests mirror each other (same bounds block, same 321). With getNameWidths from main doing the same read, the file now has three such helpers. Moving the helper up to the outer describe and looping over { index, short, long } would keep future fixes in one place. E2eFilterBarMultiSelectPipeTruncation also mirrors E2eFilterBarPipeTruncation (same template minus the kbq-filters line). Its pipes could live in the existing fixture instead of a second component, route and describe, though the existing tests use .first(), so their indices would need adjusting.
There was a problem hiding this comment.
Вынес getWidths на уровень внешнего describe и убрал его дубликаты; максимум также читается общим helper из computed style. Отдельный multi-select fixture оставил: в нём три стабильных состояния для проверки short/long/medium и тултипа, тогда как существующий fixture проверяет общий overflow и saved-filter name.
| > | ||
| <span #kbqTitleText class="kbq-pipe__name">{{ data.name }}</span> | ||
| <span #kbqTitleText class="kbq-pipe__value" [class.kbq-pipe__value_empty]="!formattedValue"> | ||
| <span #kbqTitleText kbqPipeMinWidth class="kbq-pipe__name">{{ data.name }}</span> |
There was a problem hiding this comment.
Each added kbqPipeMinWidth re-measures on every filter change, even text that can't change.
On every filterBar.filter() emission, each instance schedules its own setTimeout, reads innerText (which forces layout when the DOM is dirty) and calls markForCheck() unconditionally. Under zone.js that usually adds one change-detection pass per timer. This PR adds up to 7 instances per bar: the multi-select name and value, the multi-tree-select value, and the date and datetime names and values. data.name never changes while a pipe exists, yet every option toggle re-measures it. Measuring names once, or batching reads and writes (e.g. afterRenderEffect with earlyRead/write) and skipping markForCheck when nothing changed, would avoid this. The CSS approach from the pipe-date.html comment would remove it entirely.
There was a problem hiding this comment.
Убрал kbqPipeMinWidth из встроенных pipe, поэтому таймеры, повторное чтение innerText и markForCheck на каждое изменение filter больше не создаются. Локальный KbqPipeTitle реагирует только на resize/content changes и непосредственно перед focus/hover; его состояния покрыты unit-тестами.
Replace character-count based minimum widths with shrinkable max-content grid tracks so pipe names and values truncate independently. Keep the complete pipe, including its clear button, within the configured maximum without clamping input pipes or nested custom triggers. Add KbqPipeTitle because the shared KbqTitleDirective compares text with the complete trigger. Filter Bar divides that trigger into separate grid tracks, so a track can overflow even when its text remains narrower than the trigger. The local directive measures only Filter Bar text tracks and enables the tooltip when one is clipped, avoiding behavior changes in other components. Cover medium labels, datetime ranges, nested custom pipes, and tooltip behavior with unit and browser tests.
🚨 E2E tests failedReview the report for details. 💡 Comment |
|
/approve-snapshots |
|
🔄 Updating snapshots. |
There was a problem hiding this comment.
🔵 Needs a closer look
It reworks a shared component's layout (flex→grid) and tooltip behavior across every pipe type with visual-regression impact whose committed screenshot baselines can't be verified here, warranting human review.
Pull request overview
This PR fixes the truncation of a Filter Bar pipe's name and value (#DS-5588). Previously the name and value shared a single flex box and could be over-clipped; now each occupies its own shrinkable max-content grid track so a short part keeps its intrinsic width while the long part yields the remaining space and paints its own ellipsis. The per-pipe max-width is moved onto the whole .kbq-pipe (so it covers the clear button), and a new local KbqPipeTitle directive shows the tooltip only when a text track is actually clipped, replacing the shared KbqTitleDirective + KbqPipeMinWidth on the pipes.
Changes:
- Added
KbqPipeTitle(aKbqTooltipTriggerhost-directive) that measures each#kbqTitleTexttrack (clientWidth < scrollWidth) to toggle the tooltip; wired it into every pipe type and the module/public API. - Reworked
base-pipe.scssto a two-track grid, movedmax-widthto.kbq-pipe, and fixed the MultiTreeSelect single-selected-value rendering (button + tooltip). - Added unit tests (
pipe-title.spec.ts, multi-tree-select), a new e2e truncation scenario, browser assertions, and a dev example.
File summaries
| File | Description |
|---|---|
| pipes/pipe-title.ts | New KbqPipeTitle directive measuring per-track truncation |
| pipes/pipe-title.spec.ts | Unit tests for truncation-based tooltip enabling |
| pipes/base-pipe.scss | Flex→grid layout; max-width/min-width moved to .kbq-pipe |
| pipes/pipe-input.scss | max-width: none so input pipes escape the new pipe cap |
| pipes/base-pipe.ts | (context) KbqPipeMinWidth retained but no longer used by pipes |
| pipes/pipe-{select,multi-select,tree-select,multi-tree-select}.html | Switch to [kbqPipeTitle]; added #kbqTitleContainer (now unused) |
| pipes/pipe-{text,date,datetime,readonly}.{ts,html} | Switch to KbqPipeTitle + ignoreTooltipPointerEvents |
| pipes/pipe-multi-tree-select.html/.spec.ts | Single-selected value rendered as non-empty + tooltip fix + test |
| pipes/pipe-{select,multi-select,tree-select,datetime,date,text,multi-tree-select}.ts | Imports swapped from KbqTitleModule/KbqPipeMinWidth to KbqPipeTitle |
| filter-bar.module.ts / public-api.ts | Register/export KbqPipeTitle |
| e2e.ts / e2e.playwright-spec.ts / routes.ts | New MultiSelect truncation scenario, route, and browser assertions |
| components-dev/filter-bar/* | Dev example covering name/value length combinations |
| docs-examples/.../filter-bar-custom-pipe-example.ts | Custom pipe migrated to KbqPipeTitle |
| tools/public_api_guard/components/filter-bar.api.md | API golden updated for KbqPipeTitle |
Review details
- Files reviewed: 31/33 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
✅ Snapshots updated! |
|
закрыто в пользу #2073 - изменения действительно нужно делать в kbq-title |
Summary
Исправлена обрезка названия и значения пайпа в Filter Bar. Название и значение теперь занимают независимые сжимаемые grid-треки, а максимальная ширина применяется ко всему пайпу вместе с кнопкой очистки.
Для тултипа добавлена локальная директива
KbqPipeTitle. Она проверяет фактическую обрезку текстовых частей пайпа. ОбщаяKbqTitleDirectiveне изменялась, поэтому исправление не влияет на другие компоненты.List of notable changes:
KbqPipeTitle, потому что общаяKbqTitleDirectiveизмеряет весь триггер и не определяет обрезку отдельного grid-трека;What should reviewers focus on?
KbqPipeTitle: поведение общейKbqTitleDirectiveне изменено.Задача: DS-5588.