feat/Modal component - better typing support #2064

HugeLetters · 2023-09-21T19:14:55Z

Linked Issue

Description

This PR aims to improve typing on a modal component.

response value type is restricted based on modal type
- side note: with prompt modals the type is only a string. Even though an input type="number" can be provided through vallueAttr - Svelte will not parse its value as a number because its compiler only sees type="text" specified in the Modal component.
triggering a component modal infers props type
- also for registering components on a Modal
restrict modal trigger properties based on a modal type? For example: can't provide image if modal type is component - since it does nothing anyway, can't provide component if modal type is not a component etc.
vallueAttr is typed with HTMLInputAttributes
autocomplete for registered components keys? At first glance seems impossible - Modal component and modalStore are not coupled with each other. Only if we change the API so that you register modal components through store, not Modal component maybe?

Changsets

Instructions: Changesets automate our changelog. If you modify files in /packages/skeleton, run pnpm changeset in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should be minor while chores and bugfixes should be patch. Please prefix the changeset message with feat:, bugfix: or chore:.

Checklist

Please read and apply all contribution requirements.

This PR targets the dev branch (NEVER master)
Documentation reflects all relevant changes
Branch is prefixed with: docs/, feat/, chore/, bugfix/
Ensure Svelte and Typescript linting is current - run pnpm check
Ensure Prettier linting is current - run pnpm format
All test cases are passing - run pnpm test
Includes a changeset (if relevant; see above)

changeset-bot · 2023-09-21T19:14:58Z

⚠️ No Changeset found

Latest commit: e5c8bdd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2023-09-21T19:14:58Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
skeleton-docs	✅ Ready (Inspect)	Visit Preview	Dec 18, 2023 10:48pm

endigo9740 · 2023-09-21T19:18:21Z

@HugeLetters I've made a minor edit to the tagged issue above. Make sure to remove the {} quotes or this won't link properly and the original issue will not be closed automatically when this PR is merged.

HugeLetters · 2023-09-21T19:39:49Z

@endigo9740 could you clear something up for me please? Is the purpose of the modalStore is to just track a queue of modals if a multiple of them has been triggered (like here)? Is it also for nested modals or those are not supported? Or is there some other purpose?

endigo9740 · 2023-09-21T20:10:55Z

@HugeLetters in simplest terms the modal system is implemented globally in your app. It's implemented on-demand page to page. We use a singleton pattern for this. There's a single instance of <Modal /> that is placed in the root layout, then the queue is managed via a Svelte store. Triggering a modal is adding to the queue, closing a modal is removing from the queue. It's first in, first out.

We ship with a handful of canned options that replace native dialogs, such as alert, prompt, confirm. Then provide a means for creating your own custom modals via custom components. You can either pass them on-demand (when triggering) or register them ahead of time for reuse.

HugeLetters · 2023-09-21T20:23:04Z

@endigo9740 Yeah, I am asking specifically about this - so it's just to keep track of a modal queue? What's the purpose of queue? Is it just for sequential modals like in the link in my previous message?

endigo9740 · 2023-09-21T20:32:36Z

Correct, the role of the modal/drawer/toast stores are each to maintain their respective reactive list (read: queue) of modal instances. If multiple portions of the app trigger multiple dialogs, then they need to queue. For example, if a dialog is triggered while another dialog is being displayed.

I am asking specifically about this

Also FYI, the way these stores are initialized changed in v2 in order to prevent some known issues with SvelteKit SSR. This change was implemented by @AdrianGonz97, who worked with us on your last PR, so he would be the guy to ask if you have any questions about this.

packages/skeleton/src/lib/utilities/Modal/Modal.svelte

HugeLetters · 2023-09-22T08:41:29Z

@endigo9740 I see there is an issue that you need to keep a track of properties to pass on - here.

We can do this to at least ensure we didn't forget to pass on any props to parent but it seems not all props should be passed on - what's the criteria? Anything that's not not related to Backdrop or Transition Layer?

import type Modal from './Modal.svelte'; // you can import a module from itself
type ModalProps = Required<ComponentProps<Modal>>; // add Omit here to exclude unrelated props.
$: parent: ModalProps = { ... };

Docs claim that any Modal prop is available on the parent object - so maybe pass all of them through?

endigo9740 · 2023-09-22T15:36:15Z

@HugeLetters per the inquiry around parent. To help explain how that functions, this is a sort of duct tape fix we implemented to allow custom component modals be able to inherit and use properties set on the parent <Modal /> component. Since they're props, they may or may not be set by the end user.

I'm not sure if you've looked at the changes Svelte 5 is introducing, but my gut is telling me there will be a better way to handle this than what we're doing now. Typing for parent may be best suited to wait until then.

HugeLetters · 2023-09-22T17:51:37Z

sites/skeleton.dev/src/routes/+layout.svelte

@@ -187,7 +176,14 @@
 </svelte:head>

 <!-- Overlays -->
-<Modal components={modalComponentRegistry} />


Stuff "has" to be moved inline - otherwise you don't get the convenience of generics.

I don't particularly mind inlining props like this, but @endigo9740 may find this sacrilegious 😄. I'm fine with either.

packages/skeleton/src/lib/utilities/Modal/types.ts

HugeLetters · 2023-09-22T18:30:01Z

@AdrianGonz97 I think I'm more or less done for now - I've listed things I've done & considered.
So once you have the time please let me know if anything else could be improved.

endigo9740 · 2023-09-29T16:09:28Z

@AdrianGonz97 I think I'm more or less done for now - I've listed things I've done & considered. So once you have the time please let me know if anything else could be improved.

@AdrianGonz97 I'd welcome your review for this. If you give the thumbs up I'm happy to merge.

HugeLetters · 2023-10-06T15:43:05Z

@endigo9740 I think Adrian gave his thumbs up with a reaction 😅

AdrianGonz97 · 2023-10-06T15:51:12Z

@HugeLetters heh, sorry not quite - I'll give it a review today! 😄

endigo9740 · 2023-10-06T15:55:53Z

@AdrianGonz97 I'm running through and reviews all pending PRs today, so I'll keep an eye out for your response! Thanks!

endigo9740 · 2023-11-05T16:47:12Z

@HugeLetters sorry for the delay on this one - I think @AdrianGonz97 and myself have both been swamped. We do still intend to review this, but given the larger scope of changes this will be a bit more involved. Thanks for your patience in the meantime.

…eat/Modal-better-typing

HugeLetters · 2023-11-05T17:26:37Z

It's alright, no hurry :)

AdrianGonz97

cc: @endigo9740 @HugeLetters

Overall, I really like the changes! While I pushed some really minor changes, this PR is pretty good as-is.

Unfortunately, I believe that this PR may break some of the types for our users. For instance, in our example component ModalExampleForm.svelte, the following check needed to be added to discriminate on the Modal type, otherwise, we'd get a type-error when we try to pass flavor to response:

function onFormSubmit(): void {
	//                                                 👇
	if ($modalStore[0].response && $modalStore[0].type === 'component') 
		$modalStore[0].response(flavor);
	modalStore.close();
}

Now, whether changes to types should be considered 'breaking' is certainly a contentious topic, I believe that they should be. So while this PR is great as it is, I would hold off from merging until the next major version of Skeleton.

AdrianGonz97 · 2023-12-18T21:40:40Z

packages/skeleton/src/lib/utilities/Modal/Modal.svelte

@@ -153,18 +161,22 @@
 	}

 	function onClose(): void {
-		if ($modalStore[0].response) $modalStore[0].response(false);
+		if (currentModal.response) currentModal.response(false);


This is more of an aside than anything else:

In hindsight, it would've been preferable to return a string literal of CLOSED (or some other equivalent) instead of false. false isn't explicit enough for my liking. But alas, it would be a breaking change to change it now.

Suggested change

if (currentModal.response) currentModal.response(false);

if (currentModal.response) currentModal.response('CLOSED');

I'm fine with either - I leave it up to you guys.

AdrianGonz97 · 2023-12-18T21:43:21Z

packages/skeleton/src/lib/utilities/Modal/Modal.svelte

-		if ($modalStore[0].response) $modalStore[0].response(true);
+		if (currentModal.type !== 'confirm') return;
+
+		if (currentModal.response) currentModal.response(true);


I feel the same way about this one too.

This is one isn't breaking change actually per se since this function is only called if modalType is "confirm" - unless we consider that it's type gets mutated somewhere between rerender and a callback call(impossible basically).

We could articulate this better if we introduce Modal internal sub-components to break-up the code a little bit - and this would eliminate the need to make this check.

This is one isn't breaking change actually per se since this function is only called if modalType is "confirm"

Sorry, I should've been more specific on this one. I meant that feel the same way as I did for the aside I wrote for using string literals instead of true or false for onClose and onConfirm. We would respond with CLOSED and CONFIRMED, respectively.

AdrianGonz97 · 2023-12-18T22:04:03Z

sites/skeleton.dev/src/routes/+layout.svelte

@@ -187,7 +176,14 @@
 </svelte:head>

 <!-- Overlays -->
-<Modal components={modalComponentRegistry} />


I don't particularly mind inlining props like this, but @endigo9740 may find this sacrilegious 😄. I'm fine with either.

AdrianGonz97 · 2023-12-18T22:04:58Z

sites/skeleton.dev/src/lib/modals/examples/ModalExampleForm.svelte

@@ -19,7 +19,7 @@

 	// We've created a custom submit function to pass the response and close the modal.
 	function onFormSubmit(): void {
-		if ($modalStore[0].response) $modalStore[0].response(formData);
+		if ($modalStore[0].response && $modalStore[0].type === 'component') $modalStore[0].response(formData);


I'm afraid this may be the point where this change demonstrates that it's a breaking one for types.

endigo9740 · 2023-12-18T22:38:37Z

Now, whether changes to types should be considered 'breaking' is certainly a contentious topic, I believe that they should be. So while this PR is great as it is, I would hold off from merging until the next major version of Skeleton.

@AdrianGonz97 @HugeLetters Not to be the bearer of bad news, but if we need to treat this as a breaking change, we might not be able to move forward with this PR (yet). Skeleton v2 -> v3 will likely involve a major rewrite of this feature. If that's the case, we can continue to make updates, but only as reference for how we might handle this for the rewrite.

@HugeLetters if you would like to like to be a part of the rewrite for v3, I'm happy to bring you in for that. You and Adrian can help shape the feature as it's being written, if that makes sense. No pressure obviously, but consider it an open offer.

…eat/Modal-better-typing

HugeLetters · 2023-12-18T22:51:23Z

I'm fine if this needs to be postponed until v3 - I'm not that familiar with what should be considered breaking and not.

On one hand it is since there might exist a code base which would require some minor code changes with this update to avoid TS compilation errors(nothing changes behavior-wise).
On the other it only breaks stuff which wasn't intended to be used like that in the first place - you're not supposed to provide custom responses on confirm modals for example.

As for the my participating in implementing this feature for v3 - I would be glad to help but can't say for certain if I would have the time since I finally landed my first job so it's really hard for me to say how much spare time would I have.

endigo9740 · 2023-12-18T22:53:19Z

Fair points @HugeLetters. FYI part of the reason for the v3 rewrite is Svelte 5 support. Our hope is the new features provided will greatly simplify a lot of things, which should make introducing better type safety a bit easier. At least in theory. Fingers crossed :)

endigo9740 · 2023-12-19T20:17:51Z

@HugeLetters @AdrianGonz97 Just a quick update after sleeping on this. I think we'll go ahead and close the PR. But keep this handy as reference to inform the design process for our v3 iteration of modals. Given all the other changes coming down the pipeline, I think we'll be in for a really nice update.

Huge, sorry for this taking so long. I suspected this might be the way this goes but wanted to make sure we vetted it properly to confirm. Your contribution is appreciated and I'll definitely ping you when we get to the new modal system. If you have time to jump in and review we would sure appreciate it!

Thanks guys!

init

70d3e5e

vercel bot deployed to Preview September 21, 2023 19:16 View deployment

fixed jsdoc typo, added backdrop role

724a881

HugeLetters commented Sep 21, 2023

View reviewed changes

packages/skeleton/src/lib/utilities/Modal/Modal.svelte Outdated Show resolved Hide resolved

vercel bot deployed to Preview September 21, 2023 21:24 View deployment

response value depends on the modal type

e3b27cd

vercel bot deployed to Preview September 22, 2023 08:44 View deployment

response type refactor

880dd51

vercel bot deployed to Preview September 22, 2023 13:48 View deployment

typed component ref

40b2110

vercel bot deployed to Preview September 22, 2023 14:02 View deployment

generics for modal store trigger, input attributes type,

d24fb27

vercel bot deployed to Preview September 22, 2023 15:14 View deployment

revert a11y changes

7beb8c1

vercel bot deployed to Preview September 22, 2023 16:11 View deployment

my LSP is dying

3c4331d

vercel bot deployed to Preview September 22, 2023 17:36 View deployment

unused import

98c82cd

vercel bot deployed to Preview September 22, 2023 17:38 View deployment

cleanup

b2b43f7

vercel bot deployed to Preview September 22, 2023 17:41 View deployment

HugeLetters commented Sep 22, 2023

View reviewed changes

packages/skeleton/src/lib/utilities/Modal/types.ts Show resolved Hide resolved

endigo9740 requested a review from AdrianGonz97 September 29, 2023 16:09

endigo9740 marked this pull request as ready for review September 29, 2023 16:09

forgot to remove this

f178dd3

vercel bot deployed to Preview October 6, 2023 16:00 View deployment

Merge branch 'dev' of https://github.com/skeletonlabs/skeleton into f…

b3fcfa6

…eat/Modal-better-typing

vercel bot deployed to Preview November 5, 2023 17:28 View deployment

AdrianGonz97 added 2 commits December 18, 2023 16:57

getting rid of a couple of brackets

d50b828

import as a type

73b6c86

vercel bot deployed to Preview December 18, 2023 22:09 View deployment

AdrianGonz97 reviewed Dec 18, 2023

View reviewed changes

Merge branch 'dev' of https://github.com/skeletonlabs/skeleton into f…

e5c8bdd

…eat/Modal-better-typing

vercel bot deployed to Preview December 18, 2023 22:48 View deployment

endigo9740 marked this pull request as draft December 19, 2023 20:18

endigo9740 changed the title ~~feat/Modal component - better typing support~~ Do Not Merge: feat/Modal component - better typing support Dec 19, 2023

endigo9740 changed the title ~~Do Not Merge: feat/Modal component - better typing support~~ feat/Modal component - better typing support Dec 19, 2023

endigo9740 closed this Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/Modal component - better typing support #2064

feat/Modal component - better typing support #2064

HugeLetters commented Sep 21, 2023 •

edited

changeset-bot bot commented Sep 21, 2023 •

edited

vercel bot commented Sep 21, 2023 •

edited

endigo9740 commented Sep 21, 2023

HugeLetters commented Sep 21, 2023

endigo9740 commented Sep 21, 2023 •

edited

HugeLetters commented Sep 21, 2023 •

edited

endigo9740 commented Sep 21, 2023

HugeLetters commented Sep 22, 2023 •

edited

endigo9740 commented Sep 22, 2023

HugeLetters Sep 22, 2023

AdrianGonz97 Dec 18, 2023

HugeLetters commented Sep 22, 2023

endigo9740 commented Sep 29, 2023

HugeLetters commented Oct 6, 2023

AdrianGonz97 commented Oct 6, 2023

endigo9740 commented Oct 6, 2023

endigo9740 commented Nov 5, 2023

HugeLetters commented Nov 5, 2023

AdrianGonz97 left a comment

AdrianGonz97 Dec 18, 2023

HugeLetters Dec 18, 2023

AdrianGonz97 Dec 18, 2023

HugeLetters Dec 18, 2023 •

edited

AdrianGonz97 Dec 18, 2023

AdrianGonz97 Dec 18, 2023

AdrianGonz97 Dec 18, 2023

endigo9740 commented Dec 18, 2023 •

edited

HugeLetters commented Dec 18, 2023 •

edited

endigo9740 commented Dec 18, 2023 •

edited

endigo9740 commented Dec 19, 2023 •

edited

	if (currentModal.response) currentModal.response(false);
	if (currentModal.response) currentModal.response('CLOSED');

feat/Modal component - better typing support #2064

feat/Modal component - better typing support #2064

Conversation

HugeLetters commented Sep 21, 2023 • edited

Linked Issue

Description

Changsets

Checklist

changeset-bot bot commented Sep 21, 2023 • edited

⚠️ No Changeset found

vercel bot commented Sep 21, 2023 • edited

endigo9740 commented Sep 21, 2023

HugeLetters commented Sep 21, 2023

endigo9740 commented Sep 21, 2023 • edited

HugeLetters commented Sep 21, 2023 • edited

endigo9740 commented Sep 21, 2023

HugeLetters commented Sep 22, 2023 • edited

endigo9740 commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugeLetters commented Sep 22, 2023

endigo9740 commented Sep 29, 2023

HugeLetters commented Oct 6, 2023

AdrianGonz97 commented Oct 6, 2023

endigo9740 commented Oct 6, 2023

endigo9740 commented Nov 5, 2023

HugeLetters commented Nov 5, 2023

AdrianGonz97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HugeLetters Dec 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endigo9740 commented Dec 18, 2023 • edited

HugeLetters commented Dec 18, 2023 • edited

endigo9740 commented Dec 18, 2023 • edited

endigo9740 commented Dec 19, 2023 • edited

HugeLetters commented Sep 21, 2023 •

edited

changeset-bot bot commented Sep 21, 2023 •

edited

vercel bot commented Sep 21, 2023 •

edited

endigo9740 commented Sep 21, 2023 •

edited

HugeLetters commented Sep 21, 2023 •

edited

HugeLetters commented Sep 22, 2023 •

edited

HugeLetters Dec 18, 2023 •

edited

endigo9740 commented Dec 18, 2023 •

edited

HugeLetters commented Dec 18, 2023 •

edited

endigo9740 commented Dec 18, 2023 •

edited

endigo9740 commented Dec 19, 2023 •

edited