Allow RichTextWidget to accept TinyMCE init options #130

Merged
merged 1 commit into from Mar 21, 2013

Conversation

Projects
None yet
3 participants
@davidjb
Contributor

davidjb commented Oct 22, 2012

The new options attribute allows one to configure any arbitrary init options for TinyMCE -- eg any option from http://www.tinymce.com/wiki.php/Configuration.

Option is specified as either a dict-style (or two-tuple) structure of Python values and converted into JSON. These options are included in the JavaScript template for the widget against the TinyMCE init call.

My only suggestion beyond what's here is whether the existing TinyMCE-specific options on the widget (height, width, theme, skin) should be made to be defined as part of the options dict. Having it all as one structure is more consistent (eg all options can be set in the same place) and makes the template easier to maintain (eg just a set of 'options' to output rather than 4+ individual settings to specify and name). Backwards compatibility can be maintained by falling back to look for these 4 options where they currently exist as attributes on the widget. Thoughts?

@davidjb

This comment has been minimized.

Show comment
Hide comment
@davidjb

davidjb Mar 14, 2013

Contributor

Rebased on the latest deform master.

Contributor

davidjb commented Mar 14, 2013

Rebased on the latest deform master.

@chrisrossi

This comment has been minimized.

Show comment
Hide comment
@chrisrossi

chrisrossi Mar 19, 2013

Member

This seems to be a fundamentally solid pull request. I like the idea of ditching width, height, et al from the standard signature and making 'options' the standard way to pass all of those. I think the code can actually remain exactly as it is. The docs can be updated to reflect the 'preferred' API. The documentation for the options parameter should include documentation on the most common parameters with the link out to TinyMCE docs for extended options. Common options should include, as a minimum, the parameters which currently have their own args in the widget. Keeping the code the same and just updating the docs preserves backwards compatibility as well as sensible defaults, but allows us to publish our preferred API. Any interest in taking on the docs changes?

Member

chrisrossi commented Mar 19, 2013

This seems to be a fundamentally solid pull request. I like the idea of ditching width, height, et al from the standard signature and making 'options' the standard way to pass all of those. I think the code can actually remain exactly as it is. The docs can be updated to reflect the 'preferred' API. The documentation for the options parameter should include documentation on the most common parameters with the link out to TinyMCE docs for extended options. Common options should include, as a minimum, the parameters which currently have their own args in the widget. Keeping the code the same and just updating the docs preserves backwards compatibility as well as sensible defaults, but allows us to publish our preferred API. Any interest in taking on the docs changes?

@davidjb

This comment has been minimized.

Show comment
Hide comment
@davidjb

davidjb Mar 20, 2013

Contributor

Yep, sounds fine. I'll take care of the doc changes, but may not be able to do this for a week or so, depending on free time. The documentation is almost right to match what you've said. If someone wants to jump in ahead of me, go right ahead.

Contributor

davidjb commented Mar 20, 2013

Yep, sounds fine. I'll take care of the doc changes, but may not be able to do this for a week or so, depending on free time. The documentation is almost right to match what you've said. If someone wants to jump in ahead of me, go right ahead.

@davidjb

This comment has been minimized.

Show comment
Hide comment
@davidjb

davidjb Mar 21, 2013

Contributor

Actually, thinking about this again, the way the defaults are being supplied needs to be rethought, otherwise there's two sources of defaults (eg those hard-coded in the template, and those as now-deprecated class attributes which are still in the template) and then yet another source of options being the options 2-tuple/dict.

That's confusing as heck in my opinion so I propose the given changes - full backwards compatibility is maintained for class attributes:

  • Remove the class-level attribute specifications in the template
  • Specify all default options within a class-level attribute (except language and elements -- both of which are template dependent)
  • In the serialize function:
    • Load complete default options as a dict
    • Update default options with deprecated class-level options for height, width, skin, theme
    • Update above with contents of new options dict
    • Serialize to JSON and output

So, anyone who is still using class-level attributes (or old templates with class level attributes) will see those continue to function, all the defaults are in one place, and all overrides are in one place. Makes sense to me.

Tests included and fully documented.

Contributor

davidjb commented Mar 21, 2013

Actually, thinking about this again, the way the defaults are being supplied needs to be rethought, otherwise there's two sources of defaults (eg those hard-coded in the template, and those as now-deprecated class attributes which are still in the template) and then yet another source of options being the options 2-tuple/dict.

That's confusing as heck in my opinion so I propose the given changes - full backwards compatibility is maintained for class attributes:

  • Remove the class-level attribute specifications in the template
  • Specify all default options within a class-level attribute (except language and elements -- both of which are template dependent)
  • In the serialize function:
    • Load complete default options as a dict
    • Update default options with deprecated class-level options for height, width, skin, theme
    • Update above with contents of new options dict
    • Serialize to JSON and output

So, anyone who is still using class-level attributes (or old templates with class level attributes) will see those continue to function, all the defaults are in one place, and all overrides are in one place. Makes sense to me.

Tests included and fully documented.

@cguardia

This comment has been minimized.

Show comment
Hide comment
@cguardia

cguardia Mar 21, 2013

Member

This looks good, but we made a bunch of changes today. Can you rebase again?

Thanks a lot!

Member

cguardia commented Mar 21, 2013

This looks good, but we made a bunch of changes today. Can you rebase again?

Thanks a lot!

Allow RichTextWidget to accept options
The new ``options`` attribute allows one to configure
any arbitrary init options for TinyMCE.
@davidjb

This comment has been minimized.

Show comment
Hide comment
@davidjb

davidjb Mar 21, 2013

Contributor

Okay, rebased again. How's that?

Contributor

davidjb commented Mar 21, 2013

Okay, rebased again. How's that?

cguardia added a commit that referenced this pull request Mar 21, 2013

Merge pull request #130 from davidjb/master
Allow RichTextWidget to accept TinyMCE init options

@cguardia cguardia merged commit fae6644 into Pylons:master Mar 21, 2013

@cguardia

This comment has been minimized.

Show comment
Hide comment
@cguardia

cguardia Mar 21, 2013

Member

Cool! Thank you.

Member

cguardia commented Mar 21, 2013

Cool! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment