Skip to content
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

Remove useMultiline and multiline recipe #2551

Open
raineorshine opened this issue Nov 5, 2024 · 0 comments
Open

Remove useMultiline and multiline recipe #2551

raineorshine opened this issue Nov 5, 2024 · 0 comments
Labels
refactor Refactor without changing behavior

Comments

@raineorshine
Copy link
Contributor

raineorshine commented Nov 5, 2024

Current Behavior

Currently single line thoughts and multiline thoughts use different line-heights:

Single line thought:

/* Use line-height to vertically center the text and bullet. We cannot use padding since it messes up the selection. This needs to be overwritten on multiline elements. See ".child .editable" below. */
/* must match value used in Editable useMultiline */
lineHeight: '1.72',

Multiline thought:

lineHeight: 1.25,

This is expensive to maintain, both in terms of development effort and performance. It requires a complex hook with costly DOM measurement (https://github.com/cybersemics/em/blob/2f074c6714c4bf9e6d833d991b342a69a5dbd80c/src/components/Editable/useMultiline.ts). Since there is no event that is fired when a thought wraps, we essentially need to measure the DOM anytime anything that could affect the line-wrapping changes. (A similar strategy is still necessary to measure the height of the thoughts for the LayoutTree.) This has many edge cases and is highly error prone. I am still encountering edge cases that fail to update the line-height when needed (e.g. #2501).

Note: Distinct line-heights were originally used due to an issue with alignment and padding that could not be overcome. Apparently it affected the selection (?), but it's not clear exactly how.

Expected Behavior

Given the fragility that this architecture introduced, it is worth revisiting this problem and attempting to eliminate the single line thought line-height.

  • All thoughts should use line-height 1.25
  • Remove useMultiline
  • Remove the multiline recipe
  • The click area should not change
  • The position should not change
  • Test across platforms
  • Test across different font sizes
  • Add snapshot tests as needed
@raineorshine raineorshine added the refactor Refactor without changing behavior label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor without changing behavior
Projects
None yet
Development

No branches or pull requests

1 participant