freeman-lab / control-panel

embeddable panel of inputs for parameter setting
MIT License
238 stars 17 forks source link

Prepare control panel for the rocket launch #13

Closed dy closed 8 years ago

dy commented 8 years ago

Hi @rreusser @freeman-lab.

Here I have a list of changes discussed in #10 and related to #12. Technically you can skim through commits, they are narrative, but in case, I list the main changes here.

Please review! Looking forward to starting work on prama.

PS I think that there might be some other patches, like fixing ids, minor interactions or something, so if you don’t really mind my code style, I would really appreciate if you grant me contributor’s access to the repo and npm, to avoid delays.

dy commented 8 years ago

Any news here?

rreusser commented 8 years ago

Read through the PR and looks great to me! I noticed lots of *-range-* styles disappeared, but I trust it's for all the right reasons. 👍 Overall, lots of good cleanup that I wholeheartedly support. Trying to get through some work stuff at the moment, but will pull and try it out, hopefully later tonight. Thanks for doing this!

dy commented 8 years ago

@rreusser I found out with surprise that these styles were duplicated 3 times! So I merged them.

dy commented 8 years ago

Hi guys @rreusser @freeman-lab! I hate bugging you, but this PR grows bigger, which is not good, even though it consists of atomic changes. Could you take a glance, at least to say whether I move in proper direction. I don’t like risking making work which will be useless. Also I started using it in prama, I hoped to use npm-module instead of github dependency. Thanks!

freeman-lab commented 8 years ago

Hey @dfcreative, thanks for the work you've put into this! Have looked at the code and taken it for a spin.

A couple high-level thoughts, as I didn't get to comment further on https://github.com/freeman-lab/control-panel/issues/10.

I'm psyched that you want to use this in prama, and I see that might involve some changes. I'm actually quite wary of new features in control-panel, because I wrote it precisely to avoid the bloat I saw in things like datgui, and want to keep the API simple, codebase small, and look minimal.

Looking at this PR, I'd prefer to separate all the code clean-up (e.g. which looks fantastic overall and is super helpful!) into one PR, and then consider any new features one at a time, with the fair warning that I might be resistant!

Of what's here:

Sorry if this all sounds annoying! And if you just want to move full steam ahead, that's totally cool too! You could fork starting from here and make super-control-panel with all the new stuff for prama 😄

rreusser commented 8 years ago

@dfcreative Sorry was slow taking a look at it! Traveling at the moment (yayy montreal! :smile:) and wifi is sparse. Looks like the repo has disappeared though. Hate to think your work/time has been wasted. I like the idea in general of a lightweight drop-in for datgui, so if there's a consensus on what could lead to the perfect (or good-enough) solution here, I'm still glad to pitch in.

At a high level, FWIW, things that prevent control-panel being the perfect solution for me:

At any rate, giving prama a try and will continue to try to iterate. 👍

dy commented 8 years ago

@rreusser yeah, I thought first to follow the idea to create super-control-panel, that is why removed the repo. If you need the code, it is here. Some minor things, like fixed #12 can be cherry-picked I guess.

State control is a good idea btw, thanks!

rreusser commented 8 years ago

All good. I've been struggling to find the time to use these things as much as I'd like, so I'm (unfortunately) not in an urgent rush. It's a solution I'd like to see refined though, so just hated to think you put a bunch of effort in that didn't find the right fit.

This is not the place for it, but because I hate to open a bunch of issues on your WIP repo, two small things on prama: needs is-mobile added to deps, and had a bit of a tough time figuring out how to just put the resulting form in a div on the page. But interested to see where it goes. 👍

dy commented 8 years ago

Yeah, ok, thanks for the feedback, I am working on that!

freeman-lab commented 8 years ago

@rreusser totally agree, and didn't want the effort from @dfcreative to be wasted either! I'm also all for adding the input specific events, so will try to put that back in to control-panel, and will try to cherry pick the style and id fixes. It was just some of the other new features that I got cold feet about. Hope that was clear!

Anyway, also excited to see where these various efforts go, and really appreciate the contributions from both of you! 👍 👍