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

fix(types): missing/wrong types for modal. dropdown, popup, transition #3082

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Aug 4, 2024

Description

Some types fixes to reflect settings in modal, dropdown, popup and transition

Closes

#3081

@lubber-de lubber-de added state/awaiting-reviews Pull requests which are waiting for reviews type/types Anything related to types labels Aug 4, 2024
@lubber-de lubber-de added this to the 2.9.4 milestone Aug 4, 2024
@lubber-de
Copy link
Member Author

@prudho @KiddoV
Could you please review and test my changes? I was wondering why the original approach always uses confusing definitions like Partial<Pick<DropdownSettings, keyof DropdownSettings>>
which sounds unnecessary as just pointing to "DropdownSettings" should be enough, isn't it?
So for the transition settings in this PR, i just used the simple way and the parser does not complain, so i believe my change is good enough? Perhaps the Pick...keyof syntax is somehow needed?

@KiddoV
Copy link
Contributor

KiddoV commented Aug 7, 2024

@lubber-de
The use of Partial<Pick<DropdownSettings, keyof DropdownSettings>> is often employed to create a type that represents a subset of DropdownSettings, where all properties are optional. This is useful when you only want to update or provide some fields of the original type while leaving the others with default values.

In your case, if you use DropdownSettings directly, it would require all properties to be provided, which may not be ideal if you only want to specify a few fields and keep the rest at their default values.

I found myself doing something like this a lot:

{
    [...]
    transition: {
        showMethod: "fly left",
        hideMethod: "fly left"
        // I just leave the other fields alone and they will take their default values
    },
}

Using Partial<Pick<DropdownSettings, keyof DropdownSettings>> allows you to specify only the fields you want to change, while the rest remain optional and can default to their initial values.

Take a look at the Toast module:

type TransitionSettings = Partial<Pick<Settings.Transition, keyof Settings.Transition>>;

I think you should follow the approach used here... or maybe make TransitionSettings global for all module?

@lubber-de
Copy link
Member Author

@KiddoV Thanks for clarification 👍🏼, i just changed the PR accordingly.

@KiddoV
Copy link
Contributor

KiddoV commented Aug 8, 2024

This LGTM!

@lubber-de lubber-de changed the title fix(types): missing/wrong types for modal and transition fix(types): missing/wrong types for modal. dropdown, popup, transition Aug 8, 2024
@lubber-de lubber-de merged commit 263c192 into fomantic:develop Aug 8, 2024
10 checks passed
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Aug 8, 2024
@lubber-de lubber-de deleted the modalAndTransitionTypes branch August 8, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/types Anything related to types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants