search

sheadawson / silverstripe-shortcodable

Provides a GUI for CMS users to insert Shortcodes into the HTMLEditorField + an API for developers to define Shortcodable DataObjects and Views.
MIT License
48 stars 36 forks source link

Bug? Availability of placeholder may not be accurate #35

Closed jonom closed 8 years ago

jonom commented 8 years ago

If you add a placeholder to an existing shortcode, the placeholder doesn't automatically become visible for any existing instances of that shortcode because the shortcode will be missing the HasPlaceholder="1". Conversely, if you decide to remove a placeholder from a shortcode class, any instances that have the placeholder flag in place will attempt to load a placeholder but it will fail.

Better approach might be for the plugin to load a map of the insertable shortcode types and whether or not they have a placeholder method and reference this.

sheadawson commented 8 years ago

Have you got the latest code? I removed the HasPlaceHolder="1" approach here https://github.com/sheadawson/silverstripe-shortcodable/commit/e341fe6d14409aca79894dfd3fb3574e155e8752#diff-36c6a0646b70f2531fc24e8efeda9390L60

jonom commented 8 years ago

Yep I have the latest code - I think I see what's happening though. HasPlaceholder="1" isn't required anymore but it's still being inserted with new shortcodes because of this: https://github.com/sheadawson/silverstripe-shortcodable/blob/2.0.2/code/controllers/ShortcodableController.php#L94 so that's why I'm seeing it in the code and I made a wrong assumption about that needing to be there to get a placeholder.

The reason I'm not seeing a placeholder on shortcodes that lack that particular attribute is because the shortcode I'm using doesn't have any attributes, it just looks like [PhoneNumber]. I'm not great with regex but I think this pattern is requiring some white space after the shortcode class name, which should be optional. Just need to change the \s+ to \s* I think.

sheadawson commented 8 years ago

Aah ok, I'll see if I can touch those up this week :)

On 10 May 2016 at 11:07, Jono Menz notifications@github.com wrote:

Yep I have the latest code - I think I see what's happening though. HasPlaceholder="1" isn't required anymore but it's still being inserted with new shortcodes because of this: https://github.com/sheadawson/silverstripe-shortcodable/blob/2.0.2/code/controllers/ShortcodableController.php#L94 so that's why I'm seeing it in the code and I made a wrong assumption about that needing to be there to get a placeholder.

The reason I'm not seeing a placeholder on shortcodes that lack that particular attribute is because the shortcode I'm using doesn't have any attributes, it just looks like [PhoneNumber]. I'm not great with regex but I think this pattern https://github.com/sheadawson/silverstripe-shortcodable/blob/2.0.2/javascript/editor_plugin.js#L57 is requiring some white space after the shortcode class name, which should be optional. Just need to change the \s+ to \s* I think.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/sheadawson/silverstripe-shortcodable/issues/35#issuecomment-218015948

Shea Dawson Web Developer LiveSource

mobile +64 20 4028 5647 skype squatchnz email shea@livesource.co.nz web www.livesource.co.nz

sheadawson commented 8 years ago

Cheers yeap that fixed the regex :)

jonom commented 8 years ago

Thanks for fixing