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

Prepare control panel for the rocket launch #13

Closed
wants to merge 28 commits into from
Closed

Prepare control panel for the rocket launch #13

wants to merge 28 commits into from

Conversation

dy
Copy link

@dy dy commented Jul 8, 2016

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.

  • sizes are now in em's, dependent on the control-panel’s font-size. User can scale thing by setting control-panel’s font-size.
  • linear ranges and intervals have synced input[type=number] for value instead of div
  • mobile support #12 is solved without multirange, for the better (it works in IE!)
  • labels are now actual <label> tags pointing to proper inputs, to allow clicking on them
  • added help and input properties to components
  • removed hack font - it saves extra-request in case if user decides to use his own font. Also hack was not loading by default due to error in code (style instead of link tag). One option would be using google-fonts, but that is a separate feature.
  • some minor style/code fixes

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
Copy link
Author

dy commented Jul 8, 2016

Any news here?

@rreusser
Copy link
Contributor

rreusser commented Jul 8, 2016

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
Copy link
Author

dy commented Jul 8, 2016

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

@dy
Copy link
Author

dy commented Jul 9, 2016

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
Copy link
Owner

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 #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:

  • I like adding the ability to set a separate input callback for each input, but I'd rather call it oninput, or maybe action to match the name of the callback for the button type.
  • I'm not sure about treating text fields as proper input elements, as in the email example, it adds complexity, and currently results in some buggy UX (e.g. the refresh after a button click, clicking one button causing validation of another input).
  • Having a default type leads to potentially confusing behavior, I'd rather require that types be explicit.
  • I'm not crazy about how the help text looks, and am inclined not to add it if we don't show it.
  • Not crazy about making the numbers actual input fields, because the up/down doesn't match the look, and being able to type values can yield buggy UX (what happens if you leave it blank?)
  • I'd rather not change the style at all (spacing has changed in various places) unless really needed.

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 😄

@dy dy closed this Jul 10, 2016
@dy dy mentioned this pull request Jul 10, 2016
@rreusser
Copy link
Contributor

@dfcreative Sorry was slow taking a look at it! Traveling at the moment (yayy montreal! 😄) 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:

  • state management (either input-specific events or maybe I need to shallow-diff things and take a react-style prevState/nextState approach?)
  • themability: maybe I just need to add my own theme to better match what I'm typically aiming for

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

@dy
Copy link
Author

dy commented Jul 10, 2016

@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!

@dy dy mentioned this pull request Jul 10, 2016
@rreusser
Copy link
Contributor

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
Copy link
Author

dy commented Jul 10, 2016

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

@freeman-lab
Copy link
Owner

@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! 👍 👍

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