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

Passes editor_selector specified in configuration to jquery versions of ... #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

esmarkowski
Copy link

...tiny_mce.

The uses_jquery? method should probably be configuration controlled rather than automatic. We ran into an issue where, using jammit, the production version would use normal tinymce javascript and the development version would use the jquery version.

The main issue was that editor_selector wasn't getting passed through to the jquery selector -- so this will fix it.

@walter
Copy link
Member

walter commented Dec 4, 2011

Thanks for the work. It looks good to me.

How do the tests fare? Have you tried them?

@esmarkowski
Copy link
Author

I haven't, but given that the editor_selector is set by default to mceEditor when Configuration is created then a value should always be present.

@walter
Copy link
Member

walter commented Dec 4, 2011

Hi,

The big problem with this project is that it has test-rot which is why I encourage everyone to move to alternatives in the README.

I'm in a bit of a pickle. I don't have time to rework the tests and get them working again (they run as a plugin rather than the gem set up), but accepting patches without tests passing has put things in rough (but working) state.

If you have time, please consider having a whack at improving the test situation to help contribute to the community. I know you already have by giving your patch back to the community, but a little more effort might make the project "come back from the dead".

Best,
Walter

On Dec 5, 2011, at 12:47 PM, Spencer Markowski wrote:

I haven't, but given that the editor_selector is set by default to mceEditor when Configuration is created then a value should always be present.


Reply to this email directly or view it on GitHub:
#10 (comment)

@esmarkowski
Copy link
Author

Yeah, I hear ya. I'll take a look at the current tests over the next couple of weeks and see if there's a way to get decent coverage and move forward.

On a side note, we're using tiny_mce with and without jquery and with and without a custom value for editor_selector; The change I made works on all of those projects. :) So, maybe not the ideal test situation -- but one that allows us to keep using it for the short term.

@walter
Copy link
Member

walter commented Dec 5, 2011

Hi again,

Cool. I appreciate it!

Yeah, I'm only using with jquery at this point and I couldn't reasonably tell users it was going to work without it which is why I added the requirement in the README. Feel free to edit the README in a pull request if you get the tests working, etc.

If you are inspired to take management of the gem on, I'm also happy to hand it off to you. Let me know if that interests you.

Cheers,
Walter

On Dec 5, 2011, at 1:13 PM, Spencer Markowski wrote:

Yeah, I hear ya. I'll take a look at the current tests over the next couple of weeks and see if there's a way to get decent coverage and move forward.

On a side note, we're using tiny_mce with and without jquery and with and without a custom value for editor_selector; The change I made works on all of those projects. :) So, maybe not the ideal test situation -- but one that allows us to keep using it for the short term.


Reply to this email directly or view it on GitHub:
#10 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants