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

Refactoring & Multiline Text Type #2

Merged
merged 4 commits into from Jul 3, 2012

Conversation

@mordaroso
Copy link
Contributor

@mordaroso mordaroso commented Jun 28, 2012

Hi there

I wanted to use and extend formotion for a project. Therefore I checked the code and did some structural refactoring.

The main idea behind the refactoring was that creating and extending row types would get easier. I went a similar approach to what Formtastic does.

First I refactored following parts:

  • I moved all the row type specific parts into own classes (easy readable, extendable)
  • Use dynamic loading of available row types (convention over configuration)

And then I was able to create a new row type for multiline text:

  • Added a rowHeight parameter to the Row
  • created a new row type for multiline text as an UITextView (see screenshot)

Text Field Example
Text Field Example (Bio)

Tests are green. Documentation missing on some parts.
Before I dig deeper in to work I would like to show you the code and get your opinion on it.

@clayallsopp
Copy link
Owner

@clayallsopp clayallsopp commented Jun 28, 2012

Wow this is really awesome, much better way of organizing it. Much more extensible.

Let me play around with it but looks good so far. I'd limit this particular merge to only the refactor + the new row type, since it's getting to be a big diff right now.

@clayallsopp clayallsopp merged commit f774e62 into clayallsopp:master Jul 3, 2012
@clayallsopp
Copy link
Owner

@clayallsopp clayallsopp commented Jul 3, 2012

@mordaroso so I merged this + some changes I did (mostly moving files to a better directory structure, but also some UI changes to the TextRow to properly support placeholders). you can check the full PR here: #7

really awesome work man, thanks so much. if you plan on doing any more work on Formotion let me know and I'll just give you commit access

@mordaroso
Copy link
Contributor Author

@mordaroso mordaroso commented Jul 3, 2012

Thanks @clayallsopp. Thats really awesome. I'll check the code in detail tomorrow.

I have some other plans for formotion like

  • entry types like dates, subviews, images, etc.
  • better documentation (type properties, how to create your own types, customizing the views, etc)

I feel comfortable with creating pull requests and discuss them first. Commit access would be nice so that I'll get notifications for support.

@clayallsopp
Copy link
Owner

@clayallsopp clayallsopp commented Jul 3, 2012

Awesome, added.

(just pushed some code for picking dates ;) )

@clayallsopp

This comment has been minimized.

Copy link
Owner

@clayallsopp clayallsopp commented on lib/formotion/row_type/string_row.rb in 4495c2c Jul 12, 2012

@mordaroso any reason why there's both a StringRow and a Character, even though they're the same? Any reason not to rename Character to StringRow and then make everything else is a subclass of that?

Just thought the current structure might be confusing to people looking into it

This comment has been minimized.

Copy link
Contributor Author

@mordaroso mordaroso replied Jul 12, 2012

@clayallsopp You're right. First I wanted to extract all the parts for character based row types into an abstract class. Thats how the Character Class got born. But now the StringRow is exactly the same. Therefore I agree that we merge Character into StringRow and get rid of the abstract class (something that is not used often in ruby anyways).

@lldong
Copy link
Contributor

@lldong lldong commented Aug 6, 2012

Why the rowHeight parameter is camel-cased, it looks inconsistent with others.

@clayallsopp
Copy link
Owner

@clayallsopp clayallsopp commented Aug 6, 2012

@lldong yeah it should be changed. i'll fix it now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants