Skip to content

Conversation

@acelaya
Copy link
Collaborator

@acelaya acelaya commented Aug 14, 2025

Part of hypothesis/h#9808

This PR is an alternative to #69, with the same motivation, making it more intuitive to work with the Excerpt component, but taking a more pragmatic approach, as suggested in #69 (review)

This PR applies the next changes:

  • Excerpts can now be controlled or not, regardless of the fact that they provide internal controls to toggle the content or not. This ensures there are no confusing cases in which, depending on the value of one prop, other props end up being ignored.
    For example, previous to these changes, if inlineControls was true, collapsed and onToggleCollapsed would be ignored.
  • The props API is now defined as a union type with a discriminator prop (inlineControl: true and inlineControl: false). That allows to define other props which are only relevant for one of the two cases, like inline control customizations (texts, styles and classes).
    If one tries to accidentally provide these props for the other "branch", the static type checks will warn you.
    As the point above, this also helps preventing some props from being accidentally ignored.
  • Some props have been renamed for consistency:
    • inlineControls to inlineControl, since it handles the rendering of one control only.
    • collapse to collapsed, since it sounds more natural.
  • There's a new shadow prop that handles the rendering of the shadow that indicate there's hidden content, so there's less magic or surprising implicit behavior.
    It does keep showing by default only when inlineControl is false, to keep the behavior closer to how it currently is, but any explicitly provided value will take precedence.
  • The collapsed prop now defaults to true, which was the behavior when inlineControls was true. This makes it more consistent for both excerpts with and without inline controls.
  • The inlineControl prop is now mandatory, to make the intention more explicit. But we were already explicitly providing it in every use of the Excerpt anyway.

Todo

  • Adjust/improve tests

@acelaya acelaya marked this pull request as ready for review August 14, 2025 09:49
@acelaya acelaya requested a review from robertknight August 14, 2025 09:49
@acelaya acelaya force-pushed the excerpt-refactor branch 4 times, most recently from 23a47e6 to 84c1b8c Compare August 19, 2025 09:14
@acelaya acelaya requested a review from robertknight August 19, 2025 09:14
const isControlled = typeof collapsed === 'boolean';
// Only use this local state if Excerpt is uncontrolled
const [uncontrolledCollapsed, setUncontrolledCollapsed] = useState(
collapsed ?? true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this mirror other components which can be either controlled or uncontrolled, you'd need a defaultCollapsed prop to set the initial state in the uncontrolled case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can probably add that later if we see a need for it.

@acelaya acelaya merged commit 13ea749 into main Aug 19, 2025
2 checks passed
@acelaya acelaya deleted the excerpt-refactor branch August 19, 2025 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants