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

feat: add new changeSetting() function #294

Closed

Conversation

ghiscoding
Copy link
Contributor

@ghiscoding ghiscoding commented Aug 17, 2024

  • it's meant to be used to change a single setting at a time, for example calendar.changeSetting('range', { min: 'today' });
  • this is partially related to set() #273, I'm not sure if that is what you had in mind to close the issue.
    • also related to 3.0 Roadmap most probably
  • we can also see below that the method also has intelliSense and will show the user which value is available depending on the setting name provided

image

NOTE

I saw your code suggestion

function changeSetting(calendar: VanillaCalendar, settings: ISettings) {
  calendar.settings = { ...calendar.settings, ...settings };
}

but that might not always be a good approach because setting properties that are objects (like IRange, ISelected, ...) will be shallow merged. For example if you have 2 calendar instances and you make a change with the function you suggested, then that will change the setting on both instances

const options1 = { input: true };
const options2 = { input: true };
const calendar1 = new VanillaCalendar('#calendar-div1', options1);
const calendar2 = new VanillaCalendar('#calendar-div2', options2);
calendar1.changeSetting('range', { min: 'today' });

// with shallow copy, options1 will equal options2

... so a deep merge is better and also note that the deep merge is also extending any object properties, I gave an example in the docs :)

- for example `calendar.changeSetting('range', { min: 'today' });`
@ghiscoding
Copy link
Contributor Author

ghiscoding commented Aug 17, 2024

This PR is actually just a suggestion and I don't necessarily need it myself, though I think it is good to have. It also supports VSCode intelliSense as shown above. I did see this request came up in #273 and also in your Roadmap 3.0, so I figured I could create this PR. If you have something in mind then feel free to modify it the way you want. Cheers

@uvarov-frontend
Copy link
Owner

As far as I can see from the code, this method allows you to change only the settings section, I will add the code so that this allows you to change any options in the calendar.

@ghiscoding
Copy link
Contributor Author

I went this way because I thought that only settings can be changed on the calendar instance? The other options are only usable when creating the object, isn't it?

@uvarov-frontend
Copy link
Owner

In addition to the «settings», it should be possible to change the «pop-ups» and basic calendar settings.

@uvarov-frontend
Copy link
Owner

Will be added in v3.0.0 #293

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.

2 participants