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

date picker: use react hooks for ui state #211

Closed
wants to merge 1 commit into from
Closed

date picker: use react hooks for ui state #211

wants to merge 1 commit into from

Conversation

stroborobo
Copy link
Contributor

Fixes: #101

@stroborobo
Copy link
Contributor Author

While thinking about it now, I'm not sure the custom render function is really flexible enough. It solves the outer layout, but wouldn't help if you'd need an is-danger class on the input for example. Maybe letting the caller pass additional Input.Options? Or do you think this is out of scope anyways, then I'll remove it, having a few smaller functions makes this easier already.

@stroborobo
Copy link
Contributor Author

Removed the renderer thing

@stroborobo stroborobo changed the title date picker: use react hooks for ui state + add support for alternative renders date picker: use react hooks for ui state May 7, 2019
@MangelMaxime
Copy link
Collaborator

Thank you for the PR :)

I am not sure yet when I will have time to look at it. Because there is (for me) a lot implied by using Hook in the core library. :)

@stroborobo
Copy link
Contributor Author

If you decide against hooks I can change it to have split messages, too. So one for DatePicker State and one for the actual value. It's just the two-in-one message we currently have that I'd like to change, so the change message actually means a change.

@MangelMaxime
Copy link
Collaborator

Ah so your "problem" is because I am using a tuple for handling both the state and the selected date?

@stroborobo
Copy link
Contributor Author

Yes, in my current app I have to do some lookups for a date change, so either I'm calling things more often than needed using the current message or I have to check for changes in the change message, which feels strange.

@Nhowka
Copy link

Nhowka commented May 7, 2019

For me, I would take all messages and just have it take a DateTime option -> unit that would be the dispatcher for date changes so anyone would just have their own message instead of treating a new message from a third-party library.

So they would use it as

let view model dispatch =
    DatePicker.View.root pickerConfig model.CurrentDate (MyDateChangingMsg >> dispatch)

The Config<'Msg> also wouldn't need to know about the 'Msg, just having the style and localization would be enough.

@stroborobo
Copy link
Contributor Author

That's actually a very good idea! :)

@MangelMaxime
Copy link
Collaborator

Hello, for information

On my machine, I have an implement of the calendar which is independents of bulma-calendar because as you saw we are using version 0.1.7 while the library is in version 6 something.

The reason for that, is because they change a lot of the classes and also they want the user to consume the calendar via their JavaScript API which is not really the Elmish way to do thing.

I will try to finish my calendar CSS implementation and take all the ideas from this PR to implement the next version of the calendar which should be more robust and flexible.

@stroborobo
Copy link
Contributor Author

Sounds great, thanks for the update! I guess we can close this PR then?

@MangelMaxime
Copy link
Collaborator

Yes I guess so

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.

Explore converting Fulma.Elmish to using stateful component
3 participants