Skip to content

Conversation

@Tumph
Copy link

@Tumph Tumph commented Nov 18, 2025

Background

Resolves shop/issues-sidekick/issues/#2055

Improves the TypeScript definitions for the Modal component's overlay control methods.

Problem

The documentation for modal overlay methods (hideOverlay, showOverlay, and toggleOverlay) lacks clarity on how these methods should be used programmatically versus through user interactions..

Solution

This PR updates the TypeScript definitions for the Modal component by moving the overlay control methods (hideOverlay, showOverlay, and toggleOverlay) from the excluded props list to be properly defined as methods with detailed JSDoc comments.

The added documentation clarifies that these methods are meant to be called imperatively on the modal element instance (via refs) rather than being passed as props. It also provides guidance on the proper usage patterns for controlling modal visibility through user interactions.

🎩

  • Verify that TypeScript correctly recognizes these methods when using refs with Modal components
  • Check that the documentation appears correctly in IDE tooltips

Checklist

  • I have 🎩'd these changes
  • I have updated relevant documentation

Copy link
Author

Tumph commented Nov 18, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Tumph Tumph changed the title updated modal docs for app gen [App Gen]updated modal docs for app gen Nov 18, 2025
@Tumph Tumph changed the title [App Gen]updated modal docs for app gen [App Gen] Updated modal docs for app gen Nov 18, 2025
@Tumph Tumph changed the title [App Gen] Updated modal docs for app gen [App Gen] Updated Docs For Modal Bug in App Gen Nov 18, 2025
@Tumph Tumph marked this pull request as ready for review November 18, 2025 22:20
@github-actions
Copy link
Contributor

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset directory. If the changes are user-facing and should cause a version bump, run yarn changeset to track your changes and include them in the next release CHANGELOG. If you are making simple updates to repo configuration, examples, or documentation, you do not need to add a changeset.

@Tumph Tumph force-pushed the ag/modal-docs-fix-for-app-gen branch from fad53a5 to 6dfca9b Compare November 18, 2025 22:23
@Tumph Tumph requested a review from davebcn87 November 19, 2025 19:21
Copy link
Contributor

@davebcn87 davebcn87 left a comment

Choose a reason for hiding this comment

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

I don't think that will work, that typescript definition only affects the docs, but the real component won't have those methods.

Let's discuss possible other solutions.

@spiderbites
Copy link

I don't think that will work, that typescript definition only affects the docs, but the real component won't have those methods.

Let's discuss possible other solutions.

Correct me if I'm wrong but the real component does have these methods. They weren't added here, just documented

Copy link
Author

Tumph commented Nov 20, 2025

Yeah we just want it to use the methods properly, rather than incorrectly. Here is the diff before and after this change: https://docs.google.com/document/d/10NzJe2FEFfL0g4xrWIE8sMDRmZq111RWD6OFYCfqecc/edit?tab=t.0

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.

4 participants