search

voodoos / bs-semantic-ui-react

Bucklescript bindings for Semantic UI React
MIT License
6 stars 2 forks source link

Binding for Message component #1

Closed bojant987 closed 6 years ago

bojant987 commented 6 years ago

Hope everything is in order. I'm fairly new to both Reason and OCaml. Regards

voodoos commented 6 years ago

Hi Bojan, thanks for sharing your improvements !

Your code looks right to me, the only thing I noticed is that you named the Message sub-modules MessageContent, MessageHeader etc. But because these are sub-modules of Message you need to use Message.MessageContent to access them. It's less redundant do call them Content, Header etc and consistent with the rest of the library.

Furthermore :

[@bs.module "semantic-ui-react"] [@bs.scope "Message"] external react : ReasonReact.reactClass = "MessageContent";

Should be:

[@bs.module "semantic-ui-react"] [@bs.scope "Message"] external react : ReasonReact.reactClass = "Content";

I will test the component more thoroughly tomorrow or Monday and then merge the pull request.

bojant987 commented 6 years ago

Yeah, that makes sense. I renamed them. Sorry for two commits, first one was accidental :)

voodoos commented 6 years ago

Done !

I will try to add some tests and CI to the repo soon to automate some of this process...

bojant987 commented 6 years ago

Cool! Good luck! Hopefully i'll find time to help with some of the tests.