-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat(combobox): display overflowing selected content in a tooltip #9014
feat(combobox): display overflowing selected content in a tooltip #9014
Conversation
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.
Initial comments.
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
Here's the changes I was working with: diff --git forkSrcPrefix/packages/calcite-components/src/components/combobox/combobox.e2e.ts forkDstPrefix/packages/calcite-components/src/components/combobox/combobox.e2e.ts
index 4048a7551c04aba976951ec5dfe6eeda2fa46c85..efedb22d6277128f74fc05a2cc952d0215cec23e 100644
--- forkSrcPrefix/packages/calcite-components/src/components/combobox/combobox.e2e.ts
+++ forkDstPrefix/packages/calcite-components/src/components/combobox/combobox.e2e.ts
@@ -2009,26 +2009,22 @@ describe("calcite-combobox", () => {
const longValue = "Natural Resources Including a Comprehensive List of Resources to Protect or Plunder";
const page = await newE2EPage();
await page.setContent(html`
- <calcite-combobox open placeholder="Select a field" selection-mode="single">
- <calcite-combobox-item value="NaturalResources" text-label=${longValue}></calcite-combobox-item>
- <calcite-combobox-item value="Agriculture" text-label="Agriculture"></calcite-combobox-item>
- <calcite-combobox-item value="Forestry" text-label="Forestry"></calcite-combobox-item>
- <calcite-combobox-item value="Mining" text-label="Mining"></calcite-combobox-item>
- <calcite-combobox-item value="Business" text-label="Business"></calcite-combobox-item>
- </calcite-combobox>
+ <div style="width:200px;">
+ <calcite-combobox placeholder="Select a field" selection-mode="single">
+ <calcite-combobox-item selected value="NaturalResources" text-label="${longValue}"></calcite-combobox-item>
+ <calcite-combobox-item value="Agriculture" text-label="Agriculture"></calcite-combobox-item>
+ <calcite-combobox-item value="Forestry" text-label="Forestry"></calcite-combobox-item>
+ <calcite-combobox-item value="Mining" text-label="Mining"></calcite-combobox-item>
+ <calcite-combobox-item value="Business" text-label="Business"></calcite-combobox-item>
+ </calcite-combobox>
+ </div>
`);
await page.waitForChanges();
- const combobox = await page.find("calcite-combobox");
- const firstItem = await combobox.find("calcite-combobox-item[value=NaturalResources]");
-
- console.log("firstItem", firstItem);
- await firstItem.click();
const inputWrap = await page.find(`calcite-combobox >>> .${CSS.inputWrap}`);
- expect(inputWrap).toHaveAttribute("title");
- expect(inputWrap.textContent.length).toBeLessThan(longValue.length);
- expect(inputWrap.getAttribute("title")).toEqual(longValue);
+ expect(inputWrap.innerText.length).toBeLessThan(longValue.length);
+ expect(inputWrap.title).toEqual(longValue);
});
});
diff --git forkSrcPrefix/packages/calcite-components/src/components/combobox/resources.ts forkDstPrefix/packages/calcite-components/src/components/combobox/resources.ts
index d4efe67ab8c2ae47dea2dca7145be9d2a6b683c4..e5e972a45f665a8984857f7b3863a0a80c368776 100644
--- forkSrcPrefix/packages/calcite-components/src/components/combobox/resources.ts
+++ forkDstPrefix/packages/calcite-components/src/components/combobox/resources.ts
@@ -5,6 +5,7 @@ export const ComboboxChildSelector = `${ComboboxItem}, ${ComboboxItemGroup}`;
export const CSS = {
chipInvisible: "chip--invisible",
inputWrap: "input-wrap",
+ inputWrapSingle: "input-wrap--single",
selectionDisplayFit: "selection-display-fit",
selectionDisplaySingle: "selection-display-single",
listContainer: "list-container",
diff --git forkSrcPrefix/packages/calcite-components/src/components/combobox/combobox.tsx forkDstPrefix/packages/calcite-components/src/components/combobox/combobox.tsx
index e50420dfe42d5070d16a9c74f3499a97871bcc9c..cef51f93972f577d82d8e99e67a831983f81c9ae 100644
--- forkSrcPrefix/packages/calcite-components/src/components/combobox/combobox.tsx
+++ forkDstPrefix/packages/calcite-components/src/components/combobox/combobox.tsx
@@ -463,7 +463,6 @@ export class Combobox
afterConnectDefaultValueSet(this, this.getValue());
connectFloatingUI(this, this.referenceEl, this.floatingEl);
setComponentLoaded(this);
- this.setTooltipText();
}
componentDidRender(): void {
@@ -774,10 +773,13 @@ export class Combobox
private setTooltipText = (): void => {
const { textLabelEl } = this;
- if (textLabelEl) {
- this.tooltipText =
- textLabelEl.offsetWidth < textLabelEl.scrollWidth ? this.textLabelEl.innerText : null;
+
+ if (!textLabelEl) {
+ return;
}
+
+ this.tooltipText =
+ textLabelEl.offsetWidth < textLabelEl.scrollWidth ? textLabelEl.innerText : null;
};
private setTextInputWrapEl = (el: HTMLInputElement): void => {
@@ -785,6 +787,7 @@ export class Combobox
if (el) {
this.resizeObserver?.observe(el);
+ this.setTooltipText();
}
};
@@ -1551,7 +1554,7 @@ export class Combobox
}
private renderInput(): VNode {
- const { guid, disabled, placeholder, selectionMode, selectedItems, open } = this;
+ const { guid, disabled, placeholder, selectionMode, selectedItems, open, tooltipText } = this;
const single = isSingleLike(selectionMode);
const selectedItem = selectedItems[0];
const showLabel = !open && single && !!selectedItem;
@@ -1559,10 +1562,10 @@ export class Combobox
return (
<span
class={{
- "input-wrap": true,
- "input-wrap--single": single,
+ [CSS.inputWrap]: true,
+ [CSS.inputWrapSingle]: single,
}}
- title={this.tooltipText}
+ title={tooltipText}
// eslint-disable-next-line react/jsx-sort-props -- ref should be last so node attrs/props are in sync (see https://github.com/Esri/calcite-design-system/pull/6530)
ref={this.setTextInputWrapEl}
>
@@ -1597,6 +1600,8 @@ export class Combobox
onInput={this.inputHandler}
placeholder={placeholder}
type="text"
+ // eslint-disable-next-line react/jsx-sort-props -- ref should be last so node attrs/props are in sync (see https://github.com/Esri/calcite-design-system/pull/6530)
+ ref={(el) => (this.textInput = el as HTMLInputElement)}
/>
</span>
);
|
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.
comments :)
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/combobox/combobox.tsx
Outdated
Show resolved
Hide resolved
a8cd241
to
c6d7476
Compare
|
||
requestAnimationFrame(() => { | ||
this.tooltipText = | ||
textLabelEl.offsetWidth < textLabelEl.scrollWidth ? textLabelEl.innerText : null; |
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 would compare the actual text width using this util with the computed width of the text's container. Also, rather than making a separate @State
property for the tooltip text, just use the selected item's display value (which is verbatim what the tooltip would be) and in the render method, just conditionally render the title
attribute based on if the value is truncated or not.
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 don't think that util makes sense in this scenario. We just want to know if the offsetWidth is less than the scrollWidth which is simpler than creating a canvas and measuring.
I do agree that the @State()
isn't ideal. It could probably just be a boolean for whether to render the title or not and use the selected items display value. I think it still needs a state because the render method wouldn't update when the resize observer fires unless it tells some state to change.
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.
Note: Button seems to do the same logic so a util that handles this might be useful. Something like hasScrollbar(el);
calcite-design-system/packages/calcite-components/src/components/button/button.tsx
Lines 414 to 420 in 210e2da
private setTooltipText = (): void => { | |
const { contentEl } = this; | |
if (contentEl) { | |
this.tooltipText = | |
contentEl.offsetWidth < contentEl.scrollWidth ? this.el.innerText || null : null; | |
} | |
}; |
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 also thought, why not just always render the full title
attribute? Is the intention to not unnecessarily crowd the UI when hovering the selected item with the mouse if the full text is visible? Either way, rendering the title all the time would just be the simplest approach, no need to calculate overflow whatsoever... Just some food for thought.
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.
Yeah I thought that too. IMO that would be the simplest option. It is a tiny bit odd to conditionally render a title attribute.
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.
This came up with tab-title/dropdown truncation as well. Since truncation is happening on all long text items, we decided at that time that there was no need to have a popup with duplicate info where not necessary. The question was, what would a dropdown item look like with a title tooltip with a repeat text and calcite-tooltip?
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.
Thanks @Elijbet for the refresher! If we play this out to possible truncation behavior in places like List or Tree, I agree that keeping this conditional is a cleaner UX for the end user. If the text is short enough to be displayed then there isn't much reason to also render the title
attribute. Add a possible calcite-tooltip
on top of that and there's suddenly a lot going on. Reducing redundancy feels like a win.
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.
We've also been talking about mapping out a system-wide configurable text-overflow
. With that in mind and since, in this case, the entire text can be read by opening the dropdown, I want to put it out there that we could abandon using the title
attribute here for the time being.
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.
Ok, that makes sense to me.
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.
Closing the issue then.
@@ -531,6 +532,9 @@ export class Combobox | |||
|
|||
@State() text = ""; | |||
|
|||
/** keeps track of the tooltipText */ | |||
@State() tooltipText: string; |
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 this should just change to @State() showTooltipText = false;
No need to store the string value since its just the selected item's display value.
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.
Agreed. Storing the value is duplicative. You should be able to get the selected item's value with the element reference in the ResizeObserver callback function.
Related Issue: #7923
Summary
For text that has been truncated in
selection-mode="single"
, applytitle attribute
conditionally to reveal full text for sighted users. This frees up thecalcite-tooltip
to provide additional context.Added an e2e test to ensure the long text is truncated and the title attribute is added.