-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fix #543; divider width should match largest sibling #2108
Fix #543; divider width should match largest sibling #2108
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.
Looks good, just seeing a few issues.
- The width of the divider should be equal to the width of the widest sibling, not cousins. Sorry that was definitely not clear in the original issue, as the screenshot depicted a different case (see [3] below).
Here, the first divider should only be as long as b2
, not This is a longer thought
:
![image](https://private-user-images.githubusercontent.com/750276/345503004-c50b61b0-dfd4-402d-b6d4-4e61b671adc3.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYwMDg3NjIsIm5iZiI6MTcyNjAwODQ2MiwicGF0aCI6Ii83NTAyNzYvMzQ1NTAzMDA0LWM1MGI2MWIwLWRmZDQtNDAyZC1iNmQ0LTRlNjFiNjcxYWRjMy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTEwJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkxMFQyMjQ3NDJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1iM2Q5YzYwYWZlYWQwNDk4ODI2OGQwYzI1MmQyMDBmZTQ0MjhkNDFlMzZkMmYxNDNkY2MyODEwMDMyNzk2NWJiJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.wJgji53MsQYNRLlLVg7UWpyln5TISs3ZavmolFCAH-k)
Instead of using data-depth
, I would suggest storing the parent's Path
. That way you can uniquely identify all children of the divider's parent.
-
The divider width should update dynamically as the user types. I suggest subscribing to the editingValueStore.
-
When the divider is the only child in the second column of a table view, it should be set to the max width of all siblings and cousins (other col2 thoughts in the same table but a different row). (This was documented in the original issue here; sorry that wasn't clear from the beginning). Technically this case is what the screenshot in the original issue depicts. I will update it so it's clearer.
I would suggest storing the grandparent's Path
so that you can uniquely identify all the grandchildren (col2 thoughts) within a divider's table.
This comment was marked as resolved.
This comment was marked as resolved.
729418f
to
4f270d1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
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.
Diff looks good now, thanks!
A couple cases where the divider width is not updated. I confirmed that editingValueStore
is updated in these cases, so maybe the effect is firing before React has re-rendered. This would cause setStyle
to measure the DOM before it has been updated.
Case 1
Paste a longer thought as a cousin of the divider.
Col2, only-child divider's width is not updated.
![](https://private-user-images.githubusercontent.com/750276/346212132-27275440-7eff-4519-92ea-619be39c1203.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYwMDg3NjIsIm5iZiI6MTcyNjAwODQ2MiwicGF0aCI6Ii83NTAyNzYvMzQ2MjEyMTMyLTI3Mjc1NDQwLTdlZmYtNDUxOS05MmVhLTYxOWJlMzljMTIwMy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTEwJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkxMFQyMjQ3NDJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0xYTY1NDQyNjIwMmRhZDM4YmViODE3MmZlZWU3NDI5N2UyMGVlOTQzNWE4ZTQ0NTE5NmRlMmFlOGFlZDZjMjUwJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.vKfRFQllgqjqH7X35kvYC728j3iH3uTNtCnlYiLmP8o)
Case 2
Delete the longest thought.
Col2, only-child divider width does not update.
![](https://private-user-images.githubusercontent.com/750276/346215133-f779217d-c09f-4430-83fd-f626db60a288.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjYwMDg3NjIsIm5iZiI6MTcyNjAwODQ2MiwicGF0aCI6Ii83NTAyNzYvMzQ2MjE1MTMzLWY3NzkyMTdkLWMwOWYtNDQzMC04M2ZkLWY2MjZkYjYwYTI4OC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTEwJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkxMFQyMjQ3NDJaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1jNDcwOTU3ZGZmNDY1NGU0ZjgxNzBiOTNhZmM3N2JkYzhjYWJkZDhkOTdlOWZhYThkODU3ODFhOGI5NTZjYTkxJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.A6z0JA88s9n6pNQxyjwWLVFnWghAsnZPKTZ6zaurFT8)
src/components/Divider.tsx
Outdated
useEffect(setStyle, []) // eslint-disable-line react-hooks/exhaustive-deps | ||
editingValueStore.useEffect(setStyle) |
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's better to pass the dependencies that are expected here. No need to override the linter from what I can tell.
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 did this because we need setStyle
to be called on the initial render before editingValueStore
takes over, but providing the expected dependencies causes a loop and it gets called repeatedly. I've created an experimental branch to show this in action:
https://github.com/mathiscode/em/tree/experiment/543-divider-width-exhaustive-deps
I'm definitely open to any suggestions for a more optimal solution, but this does seem to solve the immediate problem.
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 looks like dividerRef
is not a stable reference, even if dividerRef.current
is. Learned something new! :)
So you should be able to fix it with [dividerRef.current, path]
.
Pasting/deleting should now be fixed with 01a5b25. I'll work on replacing the current approach with a more React-centric one. |
Thanks, confirmed. Using |
I noticed another case where the divider width is not updated: If you set the cursor on the divider or its parent and refresh the page, it assumes the minWidth on load. |
Good catch! 8c0df72 should resolve this - it feels a little hacky, we add an additional setTimeout in the case of isCursor, but it seems effective and shouldn't introduce any regressions. |
I'm not really comfortable with a hack here unless we have exhausted all other possibilities. The problem with introducing more timeouts is that it creates more side effects out of the React render cycle, which can lead to more problems in the future. I'm already making an exception for the other We should try to first identify the cause of the problem. Why is the width updated after navigating, but not updated after refresh? What is different about the refresh case? Usually it indicates that there is some other bit of state that needs to be subscribed to. My guess is that the divider is loaded first because it is under the cursor, and there is nothing to make it re-render once its siblings load. |
I'll find another approach and let you know what I come up with. |
I found that watching for changes to thoughtIndex is enough to set the width after the siblings are loaded in. I was also able to remove the setTimeout for the editingValueStore, and technically it will update after editing even without subscribing to editingValueStore now, but it's not immediate so keeping that effect makes for better UX. Let me know if this is a better or worse solution. 23d5f1e |
That should be fine. We could subscribe to just the thoughts that affect its width (siblings + cousins), but at the cost of selector performance. And there are probably not too many thoughts being rendered that aren't siblings or cousins anyway. |
src/components/Divider.tsx
Outdated
} | ||
} | ||
|
||
useEffect(setStyle) | ||
useEffect(setStyle, [setStyle, thoughtIndex]) |
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.
setStyle
is not a stable ref, so this will cause useEffect
to trigger on every render. This is probably masking a missing dependency.
The thoughtIndex
dependency is good because at least we're getting the additional render that is needed when the thoughts load.
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.
Oops, I'm not even sure why I did that. I pushed the fix, although it didn't affect the number of times setStyle got called, but everything seems kosher now.
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.
Confusingly, the ref container is not actually a stable reference, only the object inside:
dividerRef // unstable
dividerRef.current // stable
So the number of times setStyle
is called hasn't changed because it's still getting called on every render.
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.
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.
If dividerRef
is unstable, and dividerRef.current
is an invalid dependency, then what do we do? I must be missing something.
Maybe you can find the answer online. We can't be the first ones to run into this.
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.
Right. I'll let you know what I come up with.
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.
56171ea - After some trial and error, I think the cleanest way I can come up with is to just bring back the eslint-disable for exhaustive-deps, and at that point we don't really need to rely on the dividerRef
at all since it was just added to satisfy the eslint warning. Not having the ref as a dependency calls setStyle
the same number of times as using dividerRef.current
so we still have stable dependencies and preserve the correct behavior on page load with the divider selected. Does this seem effective enough to you, or maybe I'm missing something else?
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.
Sure, we can suppress the linter for now. There is a chance that the DOM may update and setStyle
will not be triggered, but I don't have a test case for it.
FYI It looks like the recommended solution is to switch from an object ref to a callback ref, but my experiments didn't show that it made much difference. It will be good to keep these in mind if we run into this issue in a different context though:
- https://stackoverflow.com/questions/60476155/is-it-safe-to-use-ref-current-as-useeffects-dependency-when-ref-points-to-a-dom
- https://medium.com/@teh_builder/ref-objects-inside-useeffect-hooks-eb7c15198780
- https://legacy.reactjs.org/docs/hooks-faq.html?source=post_page-----eb7c15198780--------------------------------#how-can-i-measure-a-dom-node
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 added a comment in 7e7e4b5 so that future developers can understand why we suppressed the linter.
I'm going to merge this, but I'll note the limitations of this implementation here:
|
This PR fixes #543 causing the divider to now be the same width as the largest sibling.