Code review

[Shmew]

@cmeeren @Zaid-Ajaj

When you guys have time I'd appreciate any feedback before I finish the documentation.

[cmeeren]

Make sure to test imports like

https://github.com/Shmew/Feliz.MaterialUI.Pickers/blob/fe18003354d7261f950bb4de761955046011692e/src/Hooks.fs#L10

I have occasionally had trouble with some imports, particularly multi-parameter or overloaded ones. (If they work, it's fine.)

---

Make sure to reference the correct npm package(s) here;

https://github.com/Shmew/Feliz.MaterialUI.Pickers/blob/fe18003354d7261f950bb4de761955046011692e/src/Feliz.MaterialUI.Pickers.fsproj#L30-L31

---

The link at the bottom of the readme has incorrect text.

---

Regarding ThemeOverrides.fs, is that needed since all components only have root? Can the components be used with MUI's theme overrides at all? Have you tested? (And have you tested the ThemeProps.fs stuff?)

---

AssemblyInfo.fs isn't referenced by the project file. Should it be? Also it seems to not be needed; all the info there can be placed directly in the fsproj. See e.g. here.

---

In your props, when you inherit stuff from Feliz.MaterialUI (or from other props in this library) where the inherited prop type has a corresponding module with enum props, remember to also create a corresponding "child" module and create the relevant enum prop types there inheriting each enum prop type from the "parent" module. (Was that clear?)

---

You don't need Func wrappers if there is only one parameter.

https://github.com/Shmew/Feliz.MaterialUI.Pickers/blob/fe18003354d7261f950bb4de761955046011692e/src/Props/TimePickerView.fs#L48

---

I would definitely add string overloads in places like these if it's relevant. I haven't checked the picker docs, but AFAIK anything typed as node or ReactNode can be pretty much anything (the most relevant would be string, string seq, ReactElement, ReactElement seq, int, float). If it's typed as ReactElement, then it may not accept anything else. The only way to be sure is to test.

https://github.com/Shmew/Feliz.MaterialUI.Pickers/blob/fe18003354d7261f950bb4de761955046011692e/src/Props/TimePicker.fs#L27

---

Do you have to use obj? Can you type it more strongly?

https://github.com/Shmew/Feliz.MaterialUI.Pickers/blob/fe18003354d7261f950bb4de761955046011692e/src/Props/PickerUtilsProvider.fs#L24

---

Is there anything inheritic from IReactProperty? If not, it's better to remove #. (And if you have, what and why?) The same goes for #IViewProperty etc. If it's not inherited nor designed to be, just remove the #.

https://github.com/Shmew/Feliz.MaterialUI.Pickers/blob/fe18003354d7261f950bb4de761955046011692e/src/Props/PickerModal.fs#L18

---

I assume DateIOType is compatible with DateTime? Just making sure. (Basic testing should reveal any problems.)

---

Otherwise it looks good. Note:

• The comments above may concern more places in the code than just those I have used as an example.
• I have only checked the main project, not docs/tools/etc.
• I have not checked for misspellings, incorrect/inconsistent prop names, etc.
• I think [<Erase>] should only be used when all members are inline. I have not reviewed this, just letting you know. If I'm wrong, @Zaid-Ajaj could tell you more.

[Shmew]

Yeah I need to update a lot of the metadata stuff still, I was just slapping it together first, I'll make sure that's all fixed before I publish a nuget package.

The DateIO type does not always return a DateTime if it's not date-fns, honestly it's probably worth just restricting it to that because we're essentially using our own date library in the form of .NET DateTime and date-fns does always return a DateTime. I don't see any bindings that exist of the other libraries for Fable, and have no idea why anyone would want to use them over what we already have and date-fns since that is actually compatible with DateTime, more idiomatic, and significantly smaller bundle size. There also exists a package by @zaid-ajaj, although it's currently not supporting the latest major version and thus can't be used with this library. I tried to see about updating it, but it's a lot of overloads. Might be a good usecase for using @cmeeren's generator.

If I do that I believe I could make things more restricting like locale.

The other things you mentioned were totally on point and I'll get them fixed. Thank you @cmeeren!

[Shmew]

@cmeeren

So the overrides do work and have examples of how to do it, the schema is here which is not what I had so I need to fix that.

[Shmew]

Okay I believe I've resolved most of the raised issues.

Main questions at this point:

• Can you define required dependencies of a list in Femto? e.g. packageA OR packageB
• Stuff regarding the date utils provider

[Shmew]

Went ahead and restricted it to date-fns due to the points stated above. This also makes the syntax less verbose as we don't have to always define a prop for the provider so we can just pass in the children.