-
Notifications
You must be signed in to change notification settings - Fork 1
Add Excerpt component #66
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
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.
Extracted from the client verbatim
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.
Extracted from the client, with the only difference that in there we inject the app settings
i order to resolve the inline control styles.
In here I removed any reference to the settings and added a new inlineControlsLinkStyle
optional prop that serves the same purpose.
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 brought this utility from the client, and left it as an unexposed internal hook, because it is currently used only by the Excerpt
component.
However, it is referenced in a comment inside another component, as something that could be potentially used there.
We may want to eventually expose it from here, or just duplicate the code, since it's not a huge piece of logic.
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 utility was originally written before ResizeObserver
existed, so it had a more complex implementation. With the current implementation, the code might come out better if you just used ResizeObserver
directly in the calling component.
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 think I'll keep it for now, to avoid having to refactor the Excerpt tests where observeElementSize
is currently mocked.
I'll create an issue to simplify this separately.
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.
LGTM. I noted that the code might be simplified by removing observeElementSize
and just using ResizeObserver
directly. That utility was originally more complex because it was created before ResizeObserver support became ubiquitous.
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 utility was originally written before ResizeObserver
existed, so it had a more complex implementation. With the current implementation, the code might come out better if you just used ResizeObserver
directly in the calling component.
ffa1cd8
to
5a33d39
Compare
Part of hypothesis/h#9808
Extract
Excerpt
component from the client so that we can have collapsible annotation bodies and quotes in h moderation queue.