Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Rte helpers fallback class #294

Merged
merged 3 commits into from
Sep 27, 2017
Merged

Rte helpers fallback class #294

merged 3 commits into from
Sep 27, 2017

Conversation

NathanPJF
Copy link
Contributor

What are you trying to accomplish with this PR?

Fixes #293

To prevent this silent failure/unintended behaviour, I've added a typeof check so only strings may be applied.

Checklist

For contributors:

For maintainers:

  • I have 🎩'd these changes.

@@ -15,7 +15,9 @@ slate.rte = {
* @param {string} options.tableWrapperClass - table wrapper class name
*/
wrapTable: function(options) {
options.$tables.wrap('<div class="' + options.tableWrapperClass + '"></div>');
var tableWrapperClass = typeof options.tableWrapperClass !== 'string' ? '' : options.tableWrapperClass;
Copy link
Contributor

Choose a reason for hiding this comment

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

If some passes 1234 then would result in a empty string, not '1234'. Is this okay? I'm on the fence.

Copy link
Contributor Author

@NathanPJF NathanPJF Sep 25, 2017

Choose a reason for hiding this comment

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

That's a fair concern. I can change it back to checking typeof undefined and then outputting the value always as a string.

Regarding the integer class names: Having class names that start with a number requires some work arounds to function in a stylesheet - quick codepen here. But those workarounds exist, so I shouldn't discount it.

Copy link
Contributor

@t-kelly t-kelly left a comment

Choose a reason for hiding this comment

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

LGTM

@NathanPJF NathanPJF merged commit b4ba0fe into master Sep 27, 2017
@NathanPJF NathanPJF deleted the rte-default-class branch September 27, 2017 19:05
@lock
Copy link

lock bot commented Oct 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants