smashingboxes / cardboard-admin-archive

Rails 4 CMS
http://smashingboxes.com
Other
65 stars 13 forks source link

fix “add another” wording on page edit #102

Closed joshvc closed 9 years ago

joshvc commented 9 years ago

before: "Add another true" now: "Add another question" (or whatever the part is called)

elfassy commented 9 years ago

Hi @joshvc, I think the goal here was to allow people to customize the wording. ie in the cardboard.yml, instead of repeatable: true we'd use repeatable: question I agree that a default would be preferable in cases people are still using the repeatable: true notation. Please update your code to allow both notations and I will merge it. Thanks again

joshvc commented 9 years ago

The behavior you're describing isn't documented anywhere that I found, nor is it very intuitive. I think we either need to revise the whole thing including documenting it the readme, or just let it go.

joeyjoejoejr commented 9 years ago

Yeah, I'm with @joshvc here. If we want to have customized wording I think we need to figure out how it's done. I'm in favor of having the repeatable key just be boolean and then having an optional key for custom text. Or we just let it go for now, merge Josh's change and add a feature for custom text.

elfassy commented 9 years ago

I would update the docs for now.

slideshow:
    repeatable: slide

seems simple to me. You just don't put that line all together if it's not a repeatable part. Keep in mind reverting this change might break sites using this in production.

elfassy commented 9 years ago

the backward compatible compromise would be something like

[true, false].include?(part_hash[:repeatable]) ? part_identifier.singularize : part_hash[:repeatable]
joshvc commented 9 years ago

I don't see how this could break anything in production. The only possibility is if someone, quite by accident and against any logical convention, for trees put repeatable: ficus. Now they'll get "Add another tree" instead of "Add another ficus."

joeyjoejoejr commented 9 years ago

Yeah, I don't think that backwards compatibility is an issue. We are about to make a bunch of breaking changes soon anyway. Ian is going to bump and release everything I've merged today at 0.4.0. And then pull in the rails4.2 branch as 0.5.0.

I think the big question here is whether we think that repeatable: ficus is a feature that we should support and document, or if we should do it in another way. I'm happy to gather opinions and see what other developers think.

elfassy commented 9 years ago

slideshow would give Add a new slideshow instead of Add a new slide. This was the use case that lead to this (poorly documented) change.

We could always rename the part from slideshow to slides but that's more obscure in my mind.

leonelgalan commented 9 years ago

I think we can close this issue. If we need to improve the interface/documentation for this feature, we can submit a new PR and reference this as needed.