-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[release/1.x] [table] measureTextContentWithExclusions #2027
Conversation
packages/table/src/common/utils.ts
Outdated
// hide all elements to exclude | ||
for (let i = 0; i < elementsToExclude.length; i++) { | ||
const el = elementsToExclude.item(i) as HTMLElement; | ||
el.setAttribute("style", "display: none"); |
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 should save off the old style
attribute value and set it back to what it was previously.
getComputedStyleWithExclusionsPreview: documentation | landing | table |
h/o, this won't work, give me a min |
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.
Makes sense at a glance. @tgreenwatts didn't you need this recently too?
packages/table/src/common/utils.ts
Outdated
const elementsToExclude = element.querySelectorAll(`.${CLASSNAME_EXCLUDED_FROM_TEXT_MEASUREMENT}`); | ||
|
||
// hide all elements to exclude | ||
for (let i = 0; i < elementsToExclude.length; i++) { |
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.
nit: use forEach
?
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.
you can't. it's not an array.
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, this makes a basic amount of sense, though kindof wishing there was a better way than just marking something as "ignore me" and hiding it when we calculate.
updated to do the "ignore me" part in the right spot |
packages/table/src/common/utils.ts
Outdated
const context = document.createElement("canvas").getContext("2d"); | ||
const style = getComputedStyle(element, null); | ||
const style = getComputedStyle(element); |
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.
should probably add back the second param
packages/table/src/locator.ts
Outdated
const columnHeaderAndBodyCells = this.tableElement.querySelectorAll(columnCellSelector); | ||
const columnHeaderAndBodyCells = this.tableElement.querySelectorAll(columnCellSelector) as NodeListOf< | ||
HTMLElement | ||
>; |
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 change is no longer necessary
measureTextContentWithExclusionsPreview: documentation | landing | table |
@giladgray or @themadcreator can I get a quick +1? @tgreenwatts has already tested this and it works |
undo extra typedefPreview: documentation | landing | table |
Update utils.tsPreview: documentation | landing | table |
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.
Did a quick pass, just one quick question
@@ -42,7 +44,7 @@ export const Utils = { | |||
* element's original className and any other classes passed in with variadic | |||
* arguments matching the `classNames` api. | |||
*/ | |||
assignClasses<P extends IProps>(elem: React.ReactElement<P>, ...extendedClasses: ClassValue[]) { | |||
assignClasses<P extends IProps>(elem: React.ReactElement<P>, ...extendedClasses: ClassValue[]): React.ReactNode { |
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 all the explicit return signatures?
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 find it good hygiene for public-ish APIs. Not entirely relevant to the change at hand.
Motivation
We'd like to add some text to our column headers (column descriptions) which should be shown in the header cell (as an affordance to users that they can click and view more details about the column) but not taken into account for double-click-to-resize interactions on the column.