fix #2222 regression with divider width calculations #2237
Conversation
| ...Array.from(elements).map(element => (element.querySelector('.thought') as HTMLElement).offsetWidth), | ||
| ...Array.from(elements) | ||
| .filter(element => treeNode.classList.contains('table-col1') === element.classList.contains('table-col1')) | ||
| .map(element => (element.querySelector('.thought') as HTMLElement).offsetWidth), |
There was a problem hiding this comment.
Why wouldn't it just be !element.classList.contains('table-col1')? And if that's the case, why not fine-tune the original elements selector instead of filtering it after the fact?
I can't think of a time when we need to measure col1 width, but maybe I'm missing something.
There was a problem hiding this comment.
Doing it this way causes dividers in col1, should they exist, to ignore col2 items and vice versa. It solves the problem in both directions.
There was a problem hiding this comment.
Okay, got it. Let's add a comment since it's nonobvious.
I don't love that this adds additional direct DOM access when we already have identified that this implementation suffers from too much direct DOM access. Query selectors are not captured by the type system, and they are prone to breakage if the DOM hierarchy changes. Plus, this calculation occurs on every keystroke, and the implementation is not very performance sensitive. It would have been better to identify the thoughts to be included in React, and only perform the measurement in the DOM.
I'll accept this for the sake of moving on, but I want you to be aware going forward. Thanks.
There was a problem hiding this comment.
I noticed one other problem. The col2 calculation is inadvertently being applied to dividers that are not in a table:
Steps to Reproduce
- a
- b
- =pin
- true
- This is a longer thought
- c
- ---
Current Behavior
The width of the divider is too large.
Expected Behavior
From #543:
A divider that is an only child in column 2 of a table should be left aligned with the bullets and as wide as the widest col2 thought in the same table.
The divider in this example is not in a table, so it should not take into account the width of cousins.
I recommend checking if the divider is in a table in React.
| ...Array.from(elements).map(element => (element.querySelector('.thought') as HTMLElement).offsetWidth), | ||
| ...Array.from(elements) | ||
| .filter(element => treeNode.classList.contains('table-col1') === element.classList.contains('table-col1')) | ||
| .map(element => (element.querySelector('.thought') as HTMLElement).offsetWidth), |
There was a problem hiding this comment.
Okay, got it. Let's add a comment since it's nonobvious.
I don't love that this adds additional direct DOM access when we already have identified that this implementation suffers from too much direct DOM access. Query selectors are not captured by the type system, and they are prone to breakage if the DOM hierarchy changes. Plus, this calculation occurs on every keystroke, and the implementation is not very performance sensitive. It would have been better to identify the thoughts to be included in React, and only perform the measurement in the DOM.
I'll accept this for the sake of moving on, but I want you to be aware going forward. Thanks.
| @@ -54,7 +54,9 @@ const Divider = ({ path }: { path: Path }) => { | |||
|
|
|||
| /** Find and set the max width of our selected elements. */ | |||
| const maxWidth = Math.max( | |||
| ...Array.from(elements).map(element => (element.querySelector('.thought') as HTMLElement).offsetWidth), | |||
| ...Array.from(elements) | |||
| .filter(element => treeNode.classList.contains('table-col1') === element.classList.contains('table-col1')) | |||
There was a problem hiding this comment.
treeNode.classList.contains('table-col1') can be precalculated outside the filter, or even better calculated in React:
const isTableCol1 = attributeEquals(state, head(parentOf(path)), '=view', 'Table')There was a problem hiding this comment.
Don't worry, you previously made me aware, I didn't forget. I used DOM access since it fit with the existing approach (which has yet to be refactored) as a quick patch to resolve #2222. This line would be altered along with the rest of the approach with the React-centric refactor that you had requested.
I'll get these issues resolved according to your feedback and let you know when there's an update.
There was a problem hiding this comment.
Okay, sorry for jumping the gun! Thanks.
No description provided.