search

gismofx / toast_ui.blazor_calendar

Toast UI Calendar Wrapper For Blazor
MIT License
54 stars 8 forks source link

Implementation of Time Zones #32

Closed bbaileyHOM closed 2 years ago

bbaileyHOM commented 2 years ago

Hello,

I hope that this will meet with your approval!

Changes: Added TUICalendarTimeZoneOption and TUITimeZone classes for serialization for time zones

Changed TUICalendarOptions property timezone to new time zone options class

Added TimeZones property to TUICalendarOptions for storing a list of TimeZoneInfo which will automatically populate the TUI time zones

gismofx commented 2 years ago

@bbaileyHOM this is great. Thanks for putting this together!

I have some thoughts and maybe think we should just juggle some code around.

What do you think about moving all the functionality and logic for timezone out of CalendarOptions and into of TUITimeZone class andor TUICalendarTimeZoneOption? This way there is only one "timezone" property to fill in the Options class. I think this is better for the user.

Seems like the TUICalendarTimeZoneOption class has some helpers/builders. My first thought was make 'ToTuiTimeZone' a static method in TUITimeZone and rename.. For example:

public static TUITimeZone.FromTimeZoneInfo(TimeZoneInfo timeZone, func<...., func<...... )

I see how the helper funcs are useful, but do they need to be exposed as public properties? Would you mind sharing your usage example of the funcs in the sample project?

Lastly, timezone property needs this attribute: [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]

Just some thoughts :) What do you think?

bbaileyHOM commented 2 years ago

I removed the additional code from the main options class and put it most in the converter so it is tucked a bit more out of the way and I reduced the properties to just the one. I had those in there as simpler way for the serialization.

I also made the classes that serialize for the TUI Calendar private so that they aren't visible and are only for serialization.

The expression functions previously didn't need to be public, but now due as they are accessed by the JSON converter. I also put an example with the example project.

I added the conditional ignore attribute.

I'm not sure if I'm terribly happy with the JSON Converter as it seems unnecessarily complex. I had hoped to simply read in all the JSON and to just deserialize it as the private class, but I couldn't find a way to do that. I'm not sure there is really a case for deserializing anyway.

gismofx commented 2 years ago

I removed the additional code from the main options class and put it most in the converter so it is tucked a bit more out of the way and I reduced the properties to just the one. I had those in there as simpler way for the serialization.

I also made the classes that serialize for the TUI Calendar private so that they aren't visible and are only for serialization.

The expression functions previously didn't need to be public, but now due as they are accessed by the JSON converter. I also put an example with the example project.

I added the conditional ignore attribute.

I'm not sure if I'm terribly happy with the JSON Converter as it seems unnecessarily complex. I had hoped to simply read in all the JSON and to just deserialize it as the private class, but I couldn't find a way to do that. I'm not sure there is really a case for deserializing anyway.

@bbaileyHOM Thanks. I agree...I would bring back the TUITimezone class so we don't need the custom serializer. Then we can convert a TimeZoneInfo into a TUITimeZone with an AddTimeZoneInfo helper method in TUICalendarTimeZoneOption. I think this api/library should closely parallel the TUI Calendar Api so it's clear to connect the two. Also. the TUI docs are still relevant.

gismofx commented 2 years ago

@bbaileyHOM I made some changes to the PR to match what I was thinking above. It's closer to your original commit. I'm inclined to merge. Do you have any thoughts?

gismofx commented 2 years ago

I think adding a helper function to add an Ienumerable of TimeZoneInfo could be ok.

I prefer to keep the the connection between the two libraries clear. I know there are gaps like updating settings. In those cases, I think those should be added.

Also, I want it to be "easy" to use so I'm all for improvements.

I think implementing setOptions will be very easy to do. Want to create an issue for it and I can work it? Also, check out SetCalendarOptionsAsync in the InteropService

gismofx commented 2 years ago

@bbaileyHOM I'll merge this for now and we can continue to enhance the time zone options.