-
Notifications
You must be signed in to change notification settings - Fork 4
feat(design-system): add DsTag and DsTagFilter components [AR-41960] #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(design-system): add DsTag and DsTagFilter components [AR-41960] #88
Conversation
ace9149 to
a03f7b7
Compare
078be28 to
8868ab0
Compare
8868ab0 to
c210e18
Compare
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.types.ts
Show resolved
Hide resolved
…ated dom [AR-41960]
c210e18 to
49cffd1
Compare
...esign-system/src/components/ds-form-control/stories/ds-form-control-input-number.stories.tsx
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.module.scss
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.module.scss
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.stories.tsx
Show resolved
Hide resolved
...esign-system/src/components/ds-form-control/stories/ds-form-control-input-number.stories.tsx
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.stories.tsx
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.stories.tsx
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.tsx
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/ds-tag-filter.types.ts
Outdated
Show resolved
Hide resolved
packages/design-system/src/components/ds-tag-filter/hooks/use-tag-overflow-calculation.ts
Outdated
Show resolved
Hide resolved
| * | ||
| * Measures actual element widths to determine optimal placement. | ||
| */ | ||
| export const useTagOverflowCalculation = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the internals somehow be extracted into small functions?
to make the hook easier to read
| export const Exclude: Story = { | ||
| args: { | ||
| label: 'Exclude Tag', | ||
| variant: 'exclude', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so... why do we still have a variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it as a fallback in case it appears in the app designs 🤔 Icon will always overwrite the variant. wdyt?
| const [row2TagCount, setRow2TagCount] = useState(0); | ||
| const [hasOverflow, setHasOverflow] = useState(false); | ||
|
|
||
| const calculateLayout = useCallback(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need the use callback here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One big function, with one big body. I put it in the useCallback out of habit.
edit: I will extract those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus, it is a dependencty for the useLayoutHook
| function getContainerAvailableWidth() { | ||
| const containerRect = container.getBoundingClientRect(); | ||
| const containerStyle = getComputedStyle(container); | ||
| const paddingLeft = parseFloat(containerStyle.paddingLeft) || 0; | ||
| const paddingRight = parseFloat(containerStyle.paddingRight) || 0; | ||
| return containerRect.width - paddingLeft - paddingRight; | ||
| } | ||
|
|
||
| function getElementMeasurements() { | ||
| const tags = Array.from(measurementContainer.querySelectorAll('[data-measure-tag]')); | ||
| const label = measurementContainer.querySelector('[data-measure-label]'); | ||
| const clearButton = measurementContainer.querySelector('[data-measure-clear]'); | ||
| const expandTag = measurementContainer.querySelector('[data-measure-expand]'); | ||
|
|
||
| const computedStyle = getComputedStyle(measurementContainer); | ||
| const gap = parseFloat(computedStyle.gap) || 8; | ||
|
|
||
| const tagWidths = tags.map((tag) => tag.getBoundingClientRect().width); | ||
| const labelWidth = label ? label.getBoundingClientRect().width + gap : 0; | ||
| const clearButtonWidth = clearButton ? clearButton.getBoundingClientRect().width + gap : 0; | ||
| // Fallback width for expand tag (~100px for "+99 filters" text) if measurement fails | ||
| const expandTagWidth = expandTag ? expandTag.getBoundingClientRect().width + gap : 100; | ||
|
|
||
| return { tagWidths, labelWidth, clearButtonWidth, expandTagWidth, gap }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these 2 functions be extracted outside (pass parameters to them if needed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It felt cleaner and easier to reason when everything lived inside one scope. But I can extract to separate function (2-3 arguments each)
| interface FitTagsResult { | ||
| count: number; | ||
| usedWidth: number; | ||
| } | ||
|
|
||
| /** | ||
| * Calculate how many tags fit within the available width. | ||
| * Pure function for easy testing. | ||
| */ | ||
| export function fitTagsInRow(tagWidths: number[], availableWidth: number, gap: number): FitTagsResult { | ||
| let usedWidth = 0; | ||
| let count = 0; | ||
|
|
||
| for (const tagWidth of tagWidths) { | ||
| const widthWithGap = tagWidth + (count > 0 ? gap : 0); | ||
| if (usedWidth + widthWithGap <= availableWidth) { | ||
| usedWidth += widthWithGap; | ||
| count++; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return { count, usedWidth }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe extract to its own file
if you're extracting other functions as well, I would check what's the actual final API that's being exposed the hook and test it
OR
maybe test the whole hook as one unit? is it gonna be difficult?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will extract the remaining two and see how it looks like 👍
| if (expanded) { | ||
| return { | ||
| row1TagCount, | ||
| row2TagCount: totalItems - row1TagCount, | ||
| hasOverflow: true, // Keep expand button visible to allow collapse | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this logic needs to live here?
you can do it from outside, no?
const [expanded, setExpanded] = useState(false);
const { ... } = useTagOverflowCalculation( ... );
const itemsToRender = expanded ? [ ... ] : props.items;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels easier to follow if we return the proper count from the hook, it's its job to calculate that. We only return the count of items per row. In the ds-tag-filter we take this as an input and slice the items to receive proper items
| return { | ||
| row1TagCount, | ||
| row2TagCount: totalItems - row1TagCount, | ||
| hasOverflow: true, // Keep expand button visible to allow collapse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can hasOverflow be a derived state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be, but feels harder to reason about
const hasOverflow = expanded || totalItems > row1TagCount + row2TagCount;
When we set the value explicitly:
// All tags fit in Row 1, no overflow
setRow1TagCount(row1Result.count);
setRow2TagCount(0);
setHasOverflow(false); // So, I know right away if everything fits in 1 row I expect overflow to be false
For me it feels more intuitive and intentional. I do not need to reverse the logic inside the const declaration 🤔
Performance wise it should not matter, as everything will go into one batch update, I hope 😄
https://drivenets.atlassian.net/browse/AR-39465
screen-capture.1.webm