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

Draft component designs #2

Closed
syllog1sm opened this issue Jan 9, 2014 · 19 comments
Closed

Draft component designs #2

syllog1sm opened this issue Jan 9, 2014 · 19 comments

Comments

@syllog1sm
Copy link

I've written up some thoughts on my blog about how to go about some of the components: http://clozeit.wordpress.com/

I'm pretty happy with the way I've got Buttons, Popovers and Modals working now. I'm making good progress on forms right now, I should have something up on them pretty soon.

Any thoughts?

@stevoland
Copy link
Contributor

Hi,

Really like the mixins. Do you fancy forking this project or I'm happy to contribute to yours?

@syllog1sm
Copy link
Author

I'm not really clear on the distinction --- I'm happy enough to push code to this repo. Do you think you'll have time to work on this?

@stevoland
Copy link
Contributor

I'm keen to but not sure how much time I will be able to commit. Depends if my current employer adopts React or not.

@syllog1sm
Copy link
Author

Here's a dump of the components I have (messily) implemented at the moment: https://gist.github.com/syllog1sm/8401949

I'm not very experienced with javascript, so I'm not sure how to structure a library. No idea about testing either. I can sort that out once the code's worth showing to people, but I figured I may as well show the current work, rather than sitting on it. If you want to put some of it up in a useable state, great :).

A short write up is on my blog about the tabbed area wrapper, and forms: http://clozeit.wordpress.com/

@stevoland
Copy link
Contributor

Great, thanks!

I've added the BootstrapMixin (slightly modified) and got some tests working.

@syllog1sm
Copy link
Author

Awesome!

I think there's probably enough implemented now that we might switch to an "API-first" design policy, and nail down a document specifying how we want the components to behave.

I haven't worked on a library like this, but I think that API, documentation and testing are the 3 big things. I'm pretty confident the implementation will come together pretty quickly.

Unfortunately I don't have much time to work on this for the next two weeks. So it goes...

@stevoland
Copy link
Contributor

I agree API docs first would be great.

I'm doing TDD at the moment - React components are great for that I think. I've added the size and style mixins. Think I'll setup TravisCI next then refactor ButtonDropdown to take children for the options.

@syllog1sm
Copy link
Author

Sounds great.

I've gone back and forth a bit on child passing. I see three options:

  1. Pass children as children, i.e. <Parent>...</Parent>
  2. Pass children in as a prop, so you write <Parent children={...}</Parent>
  3. Pass an array of props objects, and optionally a child class, so you write <Parent childModels={...} childClass={ChildClass}/>
  1. is the least convenient to the client in the default case, but leaves them the most flexibility, and makes it possible to modify the child properties. 1 and 2 seem equivalent to me, but there might be something I'm missing. If they are equivalent, then I'd favour 1 between the two.

@stevoland
Copy link
Contributor

Think your code's been stripped. Can you send again with backticks surrounding the code? thanks

@syllog1sm
Copy link
Author

Oops. Done.

@stevoland
Copy link
Contributor

I'd go for supporting both 1 and 3.

@stevoland
Copy link
Contributor

I realised that we can implement 1 whilst still being able to 'modify' the necessary child properties by recreating the child components and passing the props of the real children to them:

https://github.com/stevoland/react-bootstrap/blob/master/src/TabbedArea.jsx#L26

I suppose its inefficient but I think it creates the best API - how I would expect it to work. What do you think?

handleState: function (newIndex) {
  this.state({
    index: newIndex
  });
}
...
<TabbedArea activeIndex={this.state.index} onSelect={this.handleSelect}>
  <TabPane tab="Tab 1">Content</Tab>
  <TabPane tab="Tab 2">Content 2</Tab>
</TabbedArea>

@syllog1sm
Copy link
Author

Definitely sympathise with the design, but I'm worried that it'll be brittle. Pete Hunt called my attention to this:

https://github.com/facebook/react/blob/master/src/utils/cloneWithProps.js
Implementation:
https://github.com/facebook/react/blob/master/src/core/ReactPropTransferer.js

So, first up, we should use their implementation instead of your mixin. But, they seemed to indicate this was Bad Form. We should consult them on IRC.

@syllog1sm
Copy link
Author

Great work btw. I hadnt checked the project in a few days, it's coming along really nicely!

@stevoland
Copy link
Contributor

Ah, you're one step ahead as usual. Yeah cloneWithProps is what I was looking for + yeah it does seem a bit hacky but if it's going to remain supported I'd definitely like it as one option at least.

@syllog1sm
Copy link
Author

Fair enough -- gets my +1.

I think if we're going to take the idea of "just deliver the best API" seriously, then this solution is the way to go. Worst comes to worst, if React does change underneath us, we'll just change our internals.

@stevoland
Copy link
Contributor

What do you think about an option for components like TabbedArea to manage their own state or is this too much temptation to use them unwisely?

@stevoland
Copy link
Contributor

Hi,

I'm adding a LICENCE file. What's your real name to put in the copyright bit?

@syllog1sm
Copy link
Author

Heya,

Matthew Honnibal.

On Fri, Feb 7, 2014 at 8:54 PM, Stephen J. Collings <
notifications@github.com> wrote:

Hi,

I'm adding a LICENCE file. What's your real name to put in the copyright
bit?

Reply to this email directly or view it on GitHubhttps://github.com//issues/2#issuecomment-34422357
.

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

No branches or pull requests

2 participants