Table view 543 (Milestone 1) #2764

ohrytskov · 2025-01-09T22:48:12Z

#543
Built on #2763. Ignore commit 99c19d9 to review current changes only.

raineorshine · 2025-01-10T02:23:29Z

Note: Built on #2763. Ignore commit 99c19d9 to review current changes only.

ohrytskov · 2025-01-10T17:37:25Z

This PR is ready for review.

…ider same-level siblings when it's the only child, handling Table View and non-Table View scenarios.

ohrytskov · 2025-01-12T14:10:36Z

Both of the two latest commits successfully resolve the current issue. In the most recent commit, I've implemented state-based logic using selectors wherever possible instead of relying solely on DOM queries. However, since we still need to use DOM queries to measure text widths, does it make sense to combine state-based logic with selectors and DOM queries in this way? Or would it be better to use only DOM queries, as in the previous commit?

…c using selectors.

ohrytskov · 2025-01-13T12:50:36Z

Just to clarify, the two commits I'm referring to are:

Commit 919dd24: Implemented using only DOM queries.
Commit ad33c61: Switched to state-based logic using selectors wherever possible.

…tWidths helper.

trevinhofmann

Thank you for the PR, @ohrytskov!

This seems to work when pasting the given examples from the issue:

Case 1

Case 2

1. Divider widths do not update

The divider widths do not update while editing the thoughts.

Screen.Recording.2025-01-15.at.4.39.21.AM.mov

src/components/Divider.tsx

ohrytskov · 2025-01-15T22:08:33Z

Including children and thoughtsAtSameDepth in the useLayoutEffect dependency array caused an infinite re-render loop because these arrays were recreated with new references on each render. Since the hook's dependency array relies on referential equality, and the arrays were derived from selectors that created new instances of the Redux state values, the effect ran repeatedly. This triggered state updates and re-renders in a loop. To resolve this, the arrays were memoized with useMemo, ensuring their references only changed when their contents did.

… loop.

ohrytskov · 2025-01-16T13:31:41Z

@trevinhofmann, I've made the following updates:

Refactored the Divider component to use useSelectorEffect for more efficient state updates.
Memoized updateDividerWidth with useCallback to prevent redundant re-renders.

…ption.

trevinhofmann

Thank you, @ohrytskov! The latest changes resolved the issues 4. Divider widths are longer than thoughts and 5. Divider widths affected by selected thought.

I am seeing one other issue now.

6. Divider continues to grow after text wraps

When a thought's text wraps to a new line, the divider continues to grow wider. The divider should reach a maximum width.

Screen.Recording.2025-01-28.at.7.03.39.PM.mov

src/components/Divider.tsx

ohrytskov · 2025-01-29T16:07:37Z

@trevinhofmann, the PR is ready for the next review. Let me know if any further changes are needed.

…ents and using DOM getBoundingClientRect instead.

ohrytskov · 2025-01-29T23:57:44Z

The new solution now uses DOM elements to measure the maximum width of thoughts instead of relying on canvas text measurement. Since accounting for text wrapping requires accessing the actual element, we no longer need canvas for width calculations.This also simplifies the logic by returning to DOM-based measurements, making it more reliable for real-time UI updates.

trevinhofmann

Thank you, @ohrytskov!

The previous issues are all fixed, and the functionality on desktop and mobile appears to be working as expected.

raineorshine

Thank! I really like the new approach. It is lightweight since it avoids ResizeObserver and Canvas, and almost all of the logic is contained within the Divider component. Nice work.

There are a couple more dependencies that need to trigger updateDividerWidth for it to be updated when the font size changes and the window is resized:

  const fontSize = useSelector(state => state.fontSize)

  viewportStore.useSelectorEffect(updateDividerWidth, state => state.innerWidth)

Overall I like the separation between useDividerData, getThoughtWidths, and updateDividerWidth. useDividerData selects from the Redux state, getThoughtWidths interacts with the DOM, and updateDividerWidth calculates the divider width.

However, notice that updateDividerWidth is really just an extension of useDividerData, determining which thoughts' widths are relevant based on the Redux state. By splitting up useDividerData and updateDividerWidth into separate hooks, it exposes more variables than necessary to the component.

Combining useDividerData and updateDividerWidth into a single custom hook that returns ThoughtId[] would

reduce the { isOnlyChild, isTableView, children, thoughtsAtSameDepth } return object down to a single ThoughtId[]
eliminate the custom equality function
reduce the number of re-renders

Let me know if you see any issues with this suggested change!

raineorshine · 2025-01-31T08:56:06Z

src/stores/editingValueUntrimmed.ts

+ */
+const editingValueUntrimmedStore = reactMinistore<string | null>(null)
+
+export default editingValueUntrimmedStore


I agree that they can be combined, since they have the same update rates. That's fine to refactor in a separate PR. Thanks!

src/components/Editable.tsx

src/components/Divider.tsx

raineorshine · 2025-01-31T09:18:21Z

src/components/Divider.tsx

-  }
+  useEffect(() => {
+    /** Calculates and updates the Divider's width based on sibling thought widths. */
+    const updateDividerWidth = () => {


The multiple return statements and calls to setDivideWidth make this function look more complicated than it actually is. Try refactoring it so that there is a single setDividerWidth at the end and no early returns. Math.max(...widths, DIVIDER_MIN_WIDTH)) naturally returns DIVIDER_MIN_WIDTH when widths = [].

raineorshine · 2025-01-31T09:18:32Z

src/components/Divider.tsx

+  useEffect(() => {
+    /** Calculates and updates the Divider's width based on sibling thought widths. */
+    const updateDividerWidth = () => {
+      if (!dividerRef.current) return


This is an exception to my above comment. The initial short circuit to make sure the component is still mounted is fine.

raineorshine · 2025-01-31T09:23:50Z

src/components/Divider.tsx

+/**
+ * Calculates the width of multiple thoughts by measuring their rendered widths in the DOM.
+ */
+const getThoughtWidths = (thoughts: { id: ThoughtId }[]): number[] => {


Since this function only needs ids, it would be better to reduce the surface area of the parameters and just take ids: ThoughtId[].

raineorshine · 2025-01-31T09:26:43Z

src/components/Divider.tsx

+  const [dividerWidth, setDividerWidth] = useState<number>(DIVIDER_MIN_WIDTH)
+  const { isOnlyChild, isTableView, children, thoughtsAtSameDepth } = useDividerData(path)
+  const editingThoughtId = useSelector((state: State) => state.cursor && head(state.cursor))
+  const editingValueUntrimmed = editingValueUntrimmedStore.useSelector(editingValue => editingValue)


Shorthand for selecting the whole state: editingValueUntrimmedStore.useState()

raineorshine · 2025-01-31T09:46:48Z

src/components/Divider.tsx

+const getThoughtWidths = (thoughts: { id: ThoughtId }[]): number[] => {
+  return thoughts.map(thought => {
+    const innerThoughtElement = document.querySelector(
+      `[aria-label="tree-node"][data-id="${thought.id}"] [aria-label="thought"]`,


Would it work to use the existing editable element for measurement?

document.querySelector(`[aria-label="editable-${id}"]`)

raineorshine · 2025-01-31T09:49:03Z

src/components/LayoutTree.tsx

+      data-id={thoughtId}
+      data-parent-id={parentId}
+      data-depth={depth}


It looks like data-depth and data-parent-id are not used.

data-id may also be able to be removed if the sibling elements are selected by editable aria-label as mentioned above.

…ent.

ohrytskov · 2025-01-31T18:11:18Z

Thank you for your feedback, @raineorshine! I’ve carefully reviewed your suggestions and have made all the necessary changes accordingly. The PR has been updated with the requested modifications, and it is now ready for your next review. Please let me know if there are any further adjustments needed.

raineorshine

Looks great! Just a few touchups, then we should be good to merge.

raineorshine · 2025-01-31T18:27:33Z

src/components/Divider.tsx

+/** Custom hook to fetch relevant thought IDs for calculating divider width. */
+const useRelevantThoughtIds = (path: Path): ThoughtId[] => {


Let's call this useWidthDependentThoughtIds. I try to avoid words like "relevant" or "data" in variable names since they don't explain anything.

raineorshine · 2025-01-31T18:28:56Z

src/components/Divider.tsx

+
+      return relevantThoughtIds
+    },
+    (prev, next) => prev.length === next.length && prev.every((id, index) => id === next[index]),


You can use drop in shallowEqual from react-redux or isEqual from lodash rather than providing a custom equality function.

raineorshine · 2025-01-31T18:32:28Z

src/components/Divider.tsx

+      const isTableView =
+        (parentId && attributeEquals(state, parentId, '=view', 'Table')) ||
+        (grandParentId && attributeEquals(state, grandParentId, '=view', 'Table'))


You can make this a little more terse by omitting parentId && since attributeEquals handles null.

raineorshine · 2025-01-31T18:35:02Z

src/components/Divider.tsx

+    (state: State) => {
+      const parentPath = rootedParentOf(state, path)
+      const parentId = head(parentPath)
+      const grandParentPath = parentId ? rootedParentOf(state, parentPath) : []


It won't affect the behavior here, but [] is not a valid Path. It should probably be set to null if there is no parentId.

raineorshine · 2025-01-31T18:38:30Z

src/components/Divider.tsx

+        (parentId && attributeEquals(state, parentId, '=view', 'Table')) ||
+        (grandParentId && attributeEquals(state, grandParentId, '=view', 'Table'))
+
+      const relevantThoughtIds = isOnlyChild


Thanks, this is much better.

Let's call this dependentThoughtIds.

Could you also add a couple inline comments that explain the two branches?

…mprove readability.

ohrytskov · 2025-02-01T01:16:11Z

This appears to be ready for the next review—awaiting your feedback, @raineorshine.

…ve code readability.

…houghtIds.

…n useWidthDependentThoughtIds.

ohrytskov changed the title ~~Table view 543~~ Table view 543 (Milestone 1) Jan 9, 2025

ohrytskov mentioned this pull request Jan 9, 2025

Table View 1842 543 2222 (Milestone 1) #2761

Closed

ohrytskov added 3 commits January 10, 2025 00:47

Align divider with bullets.

99c19d9

Align divider with bullets.

53345cf

Refactor Divider width calculation and add parent id to TreeNode.

4a2cbb0

ohrytskov marked this pull request as ready for review January 10, 2025 17:33

ohrytskov added 2 commits January 10, 2025 19:31

Added data-depth to TreeNodes and updated Divider width logic to cons…

11d5ab7

…ider same-level siblings when it's the only child, handling Table View and non-Table View scenarios.

Ensure Divider width respects minimum value.

919dd24

Refactor Divider component: replace DOM queries with state-based logi…

ad33c61

…c using selectors.

ohrytskov added 2 commits January 13, 2025 14:49

Remove unnecessary type assertions.

4a0b47f

Refactor Divider component: extract useDividerData hook and getThough…

6365e2f

…tWidths helper.

trevinhofmann self-requested a review January 14, 2025 06:39

trevinhofmann self-assigned this Jan 14, 2025

trevinhofmann requested changes Jan 15, 2025

View reviewed changes

src/components/Divider.tsx Outdated Show resolved Hide resolved

src/components/Divider.tsx Outdated Show resolved Hide resolved

trevinhofmann assigned ohrytskov and unassigned trevinhofmann Jan 15, 2025

ohrytskov removed their assignment Jan 15, 2025

ohrytskov requested a review from trevinhofmann January 15, 2025 22:11

ohrytskov added 2 commits January 15, 2025 23:54

Optimize useLayoutEffect dependencies in Divider to prevent re-render…

16a6e26

… loop.

Refactor isTableView logic in Divider to simplify parentId computation.

fed38ee

trevinhofmann self-assigned this Jan 16, 2025

ohrytskov added 3 commits January 16, 2025 14:11

Refactor Divider component to use useSelectorEffect for state subscri…

edf436a

…ption.

Merge Table-View-1842 branch.

611311e

Merge main branch.

0514423

trevinhofmann requested changes Jan 29, 2025

View reviewed changes

src/components/Divider.tsx Outdated Show resolved Hide resolved

src/components/Divider.tsx Outdated Show resolved Hide resolved

trevinhofmann assigned ohrytskov Jan 29, 2025

ohrytskov added 2 commits January 29, 2025 15:24

Improve performance and state synchronization.

2ea45ac

Simplify canvas initialization for text width calculation.

23ba075

ohrytskov requested a review from trevinhofmann January 29, 2025 16:07

ohrytskov removed their assignment Jan 29, 2025

ohrytskov added 2 commits January 29, 2025 18:03

Simplify the text width calculation by removing canvas-based measurem…

b6621ce

…ents and using DOM getBoundingClientRect instead.

Remove redundant comments and improve readability in Divider.

cfabd05

trevinhofmann self-assigned this Jan 29, 2025

trevinhofmann approved these changes Jan 30, 2025

View reviewed changes

trevinhofmann assigned raineorshine and unassigned trevinhofmann Jan 30, 2025

raineorshine requested changes Jan 31, 2025

View reviewed changes

ohrytskov added 2 commits January 31, 2025 17:53

Refactor: Always update editingValueUntrimmedStore in Editable compon…

7d96027

…ent.

Refactor: Improve Divider component functionality and readability.

32f9c01

ohrytskov requested a review from raineorshine January 31, 2025 18:21

raineorshine requested changes Jan 31, 2025

View reviewed changes

ohrytskov added 4 commits January 31, 2025 19:02

Enhance Divider component responsiveness and dependency handling.

2bac0bb

Cleanup unused attributes in TreeNode component.

b62674c

Simplify relevant thought selection for Divider component.

b4525e2

Refactor the conditional logic in the useRelevantThoughtIds hook to i…

ed32aec

…mprove readability.

ohrytskov requested a review from raineorshine February 1, 2025 01:17

ohrytskov added 4 commits February 1, 2025 02:43

Rename useRelevantThoughtIds to useWidthDependentThoughtIds and impro…

69e7edb

…ve code readability.

Replace custom equality check with shallowEqual in useWidthDependentT…

ed7a342

…houghtIds.

Refactor: Null-check grandParentPath and simplify isTableView logic i…

3fb74a1

…n useWidthDependentThoughtIds.

Apply code formatting and organize imports for lint compliance.

cc91777

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table view 543 (Milestone 1) #2764

Table view 543 (Milestone 1) #2764

ohrytskov commented Jan 9, 2025 •

edited

Loading

raineorshine commented Jan 10, 2025

ohrytskov commented Jan 10, 2025

ohrytskov commented Jan 12, 2025

ohrytskov commented Jan 13, 2025

trevinhofmann left a comment

ohrytskov commented Jan 15, 2025

ohrytskov commented Jan 16, 2025

trevinhofmann left a comment

ohrytskov commented Jan 29, 2025

ohrytskov commented Jan 29, 2025

trevinhofmann left a comment

raineorshine left a comment •

edited

Loading

raineorshine Jan 31, 2025 •

edited

Loading

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025 •

edited

Loading

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025 •

edited

Loading

ohrytskov commented Jan 31, 2025

raineorshine left a comment

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025

raineorshine Jan 31, 2025

ohrytskov commented Feb 1, 2025

		/** Custom hook to fetch relevant thought IDs for calculating divider width. */
		const useRelevantThoughtIds = (path: Path): ThoughtId[] => {

Table view 543 (Milestone 1) #2764

Are you sure you want to change the base?

Table view 543 (Milestone 1) #2764

Conversation

ohrytskov commented Jan 9, 2025 • edited Loading

raineorshine commented Jan 10, 2025

ohrytskov commented Jan 10, 2025

ohrytskov commented Jan 12, 2025

ohrytskov commented Jan 13, 2025

trevinhofmann left a comment

Choose a reason for hiding this comment

1. Divider widths do not update

ohrytskov commented Jan 15, 2025

ohrytskov commented Jan 16, 2025

trevinhofmann left a comment

Choose a reason for hiding this comment

6. Divider continues to grow after text wraps

ohrytskov commented Jan 29, 2025

ohrytskov commented Jan 29, 2025

trevinhofmann left a comment

Choose a reason for hiding this comment

raineorshine left a comment • edited Loading

Choose a reason for hiding this comment

raineorshine Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

ohrytskov commented Jan 31, 2025

raineorshine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohrytskov commented Feb 1, 2025

ohrytskov commented Jan 9, 2025 •

edited

Loading

raineorshine left a comment •

edited

Loading

raineorshine Jan 31, 2025 •

edited

Loading

raineorshine Jan 31, 2025 •

edited

Loading

raineorshine Jan 31, 2025 •

edited

Loading