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

Improve readme #877

Merged
merged 4 commits into from Oct 16, 2013
Merged

Conversation

deivid-rodriguez
Copy link
Contributor

This is just the changes suggested in #855 by @TylerRick applied to the README

@@ -319,7 +326,7 @@ f.input :shipping_country, priority: [ "Brazil" ], collection: [ "Australia", "B

### Associations

To deal with associations, **SimpleForm** can generate select inputs, a series of radios buttons or check boxes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with space is correct. Could you change it back?

@deivid-rodriguez
Copy link
Contributor Author

So should I assume that bullet point number 1 in #855 is not agreed and just remove entirely the first of the three commits?

@rafaelfranca
Copy link
Collaborator

We are not talking about the attribute name in the text so we can't change to the version without spaces. If we will going to change it we need to change all the table and the text to talk about the attribute type. I kind agree with the the point 1, but if we are going to apply it we need to apply in all the types.

@deivid-rodriguez
Copy link
Contributor Author

I don't get it, when you say "text area" in the README, aren't you talking about the html element called "textarea"?

@rafaelfranca
Copy link
Collaborator

Yes, but we are not using the HTML attribute vocabulary (this is why it is not textarea, We are using plain english.

@deivid-rodriguez
Copy link
Contributor Author

I see. So should I just revert the entire first commit, then?

@rafaelfranca
Copy link
Collaborator

What do you think of changing all the text to use the HTML vocabulary?

@deivid-rodriguez
Copy link
Contributor Author

Sure, although I'm not an HTML vocabulary expert... :) Shall I add it to this PR?

@rafaelfranca
Copy link
Collaborator

👍

@deivid-rodriguez
Copy link
Contributor Author

Actually, what else would it need to be changed?

@rafaelfranca
Copy link
Collaborator

All the input names in the table

@deivid-rodriguez
Copy link
Contributor Author

Do you like this format?

Mapping       | Generated HTML Element               | Database Column Type
--------------+--------------------------------------+---------------------------------------------------------
boolean       | input[type="checkbox"]               | boolean
string        | input[type="text]                    | string
email         | input[type="email"]                  | string with name matching "email"
url           | input[type="url"]                    | string with name matching "url"
tel           | input[type="tel"]                    | string with name matching "phone"
password      | input[type="password"]               | string with name matching "password"
search        | input[type="search"]                 | -
text          | textarea                             | text
file          | input[type="file"]                   | string, responding to file methods
hidden        | input[type="hidden"]                 | -
integer       | input[type="number"]                 | integer
float         | input[type="number"]                 | float
decimal       | input[type="number"]                 | decimal
range         | input[type="range"]                  | -
datetime      | input[type="datetime"]               | datetime/timestamp
date          | input[type="date"]                   | date
time          | input[type="time"]                   | time
select        | select                               | belongs_to/has_many/has_and_belongs_to_many associations
radio_buttons | collection of input[type="radio"]    | belongs_to associations
check_boxes   | collection of input[type="checkbox"] | has_many/has_and_belongs_to_many associations
country       | select (countries as options)        | string with name matching "country"
time_zone     | select (timezones as options)        | string with name matching "time_zone"

@kelvinst
Copy link

@deivid-rodriguez, your suggestion is use a markdown table? I think it will be much prettier. (:

For example:

Code:

Mapping       | Generated HTML Element               | Database Column Type
--------------|--------------------------------------|---------------------------------------------------------
boolean       | input[type="checkbox"]               | boolean
string        | input[type="text]                    | string
email         | input[type="email"]                  | string with name matching "email"

Preview:

Mapping Generated HTML Element Database Column Type
boolean input[type="checkbox"] boolean
string input[type="text] string
email input[type="email"] string with name matching "email"

@deivid-rodriguez
Copy link
Contributor Author

Sorry, when I said format I meant the way of describing the html elements, namely, input[type="whatever"] etc. I really used the wrong word there... :)

Nevertheless, that table rendering is beautiful, I can add that to the PR as well... Let's see what @rafaelfranca says.

And by the way, thanks for pointing me there, I'm gonna use that markdown for my README's right now!

@rafaelfranca
Copy link
Collaborator

@deivid-rodriguez it is good to me. @carlosantoniodasilva?

@kelvinst
Copy link

Oh, sorry @deivid-rodriguez, I haven't read the previous comments right...
And you're welcome about the tip 😃

@deivid-rodriguez
Copy link
Contributor Author

Good, shall I update the table format as well or just the content?

@rafaelfranca
Copy link
Collaborator

@deivid-rodriguez it is your call. It seems good to me. Could you update the pull request so I can merge it?

@deivid-rodriguez
Copy link
Contributor Author

@rafaelfranca I added a commit a while ago, did you notice? :)

@nashby
Copy link
Collaborator

nashby commented Oct 4, 2013

@deivid-rodriguez could you please rebase it? "We can’t automatically merge this pull request." :(

@deivid-rodriguez
Copy link
Contributor Author

Sorry, I forgot about this. Is it ok now?

rafaelfranca added a commit that referenced this pull request Oct 16, 2013
@rafaelfranca rafaelfranca merged commit bc97459 into heartcombo:master Oct 16, 2013
@deivid-rodriguez deivid-rodriguez deleted the improve_readme branch October 16, 2013 13:45
@deivid-rodriguez deivid-rodriguez restored the improve_readme branch October 16, 2013 13:47
rafaelfranca added a commit that referenced this pull request Nov 26, 2013
Improve readme
Conflicts:
	README.md
rafaelfranca added a commit that referenced this pull request Nov 26, 2013
Improve readme
Conflicts:
	README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants