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

New dropdowns (working keyboard events, multi-select), checkboxes, radio groups, and examples #12

Merged
merged 10 commits into from
May 22, 2017

Conversation

tomsmalley
Copy link
Member

This is more of a request for comment, the goal is to replace the old functions entirely, but that would be a breaking change so it is up to the maintainers what the next move should be.

I've added some new dropdown functions which work fully, example at https://tomsmalley.github.io/reflex-dom-semui/. So far only static valued dropdowns are done. The old functions are quite broken: they only take into account click events, so using the keyboard can cause the state to be out of sync with the semantic-ui state. This is fixed by providing initialisation with a callback function so semantic notifies us when the value is changed - the value is set to the index of the item vector. Multi dropdowns are also included. The collection of items is now a Vector of tuples instead of a Map, this is for better user customisable ordering as well as easier indexing capability.

The ghc implementations have not been tested.

I ended up adding a dependency on lens and vector but hopefully that is okay.

I included the themes directory of semantic-ui since icons weren't working. Also the semantic library has been updated, not sure if this breaks old code.

I also suggest enabling github pages in the docs/ folder so users can see functionality more easily. I'm not sure how to build a better build system than the bash script I left in. Ideally cabal build example would do all of the work, I'm not particularly familiar with cabal though having relied on stack in the past.

Copy link
Member

@mightybyte mightybyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Great to see another person using this. See my inline comments below.


-- | Config for new semantic dropdowns
data DropdownConf t a = DropdownConf
{ _dropdownConf_setValue :: Dynamic t a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be split into initialValue :: a and setValue :: Event t a.

<$> updated (_dropdownConf_setValue config)

-- Set initial value
initialValue <- sample $ current $ _dropdownConf_setValue config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good rule of thumb with Reflex is to try to avoid calling sample. There are almost always better ways to do things. The above suggestion about splitting setValue into two fields will allow you to do this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that it was a bad thing, but it certainly led to some smells what with having to be called in m context. Thanks!

-- | Semantic-UI dropdown with static items
semUiDropdownNew
:: (MonadWidget t m, Eq a)
=> Vector (a, DropdownItemConfig m) -- ^ Items
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see much value in using Vector over something much simpler like []. Instead of depending on Vector, I'd rather depend on something that gets us more value. My recommendation would be the non-empty-zipper package. http://hackage.haskell.org/package/non-empty-zipper-0.1.0.7/docs/Data-List-NonEmptyZipper.html It allows you to guarantee that the selected item is in the list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with changing to lists, I used vector initially because I knew the indexing functions I wanted were available easily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out the NonEmptyZipper. I has some nice properties that eliminate certain classes of errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm at a loss where to apply it in this situation.

@tomsmalley
Copy link
Member Author

I've added more configurable options along with examples.

@tomsmalley
Copy link
Member Author

Another thing to note is that these implementations do not put a name for the input element, meaning native submissions won't work.
We could provide wrappers which encode the result type using aeson and put it in a hidden element. I imagine that native form submissions aren't used much though since reflex is probably used for SPAs.

@tomsmalley tomsmalley changed the title New dropdowns (working keyboard events, multi-select) and examples New dropdowns (working keyboard events, multi-select), checkboxes, radio groups, and examples May 22, 2017
@mightybyte mightybyte merged commit 68e310b into reflex-frp:master May 22, 2017
@mightybyte
Copy link
Member

Thanks! Keep up the good work!

@tomsmalley
Copy link
Member Author

Could you enable github-pages for the /docs folder? Then we can add a link in the description or readme to live examples at https://reflex-frp.github.io/reflex-dom-semui.

@ryantrinkle
Copy link
Member

ryantrinkle commented May 22, 2017 via email

@tomsmalley
Copy link
Member Author

I don't know of a good way of doing that since the examples aren't in a similar format - they're not really documentation, just elements generated by reflex-dom-semui to test functionality. I had intended users to read the source code in 'example' along with opening 'docs/index.html' in a browser. Github pages isn't entirely necessary, but it lets us show the components without users having to clone the repo (see https://tomsmalley.github.io/reflex-dom-semui/).

@mightybyte
Copy link
Member

Done. If there's a better way of integrating it, we can switch to that later.

@tomsmalley
Copy link
Member Author

Thanks!

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.

3 participants