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

Fixed #58 and Fixed #19 - throttle interfering with UI #60

Merged
merged 4 commits into from
Sep 30, 2015
Merged

Fixed #58 and Fixed #19 - throttle interfering with UI #60

merged 4 commits into from
Sep 30, 2015

Conversation

codewithtyler
Copy link
Contributor

No description provided.

@Findus23
Copy link
Contributor

I tested your fix and I am still having one issue:
The active-class is only working if I change following line https://github.com/RandomlyKnighted/bootstrap-wysiwyg/blob/underscore-fix/src/bootstrap-wysiwyg.js#L98
from

$(options.toolbarSelector,wrapper).find(toolbarBtnSelector).each(function () {

to

$(options.toolbarSelector).find(toolbarBtnSelector).each(function () {

I have only a basic knowledge of javascript, but is it possible that my HTML is malformed?
Basicly it is:

<div class="btn-toolbar" data-role="editor-toolbar" data-target="#content_de">
    <a class="btn btn-default" data-edit="format-p" title="Absätze">
        <i class="fa fa-paragraph"></i>
    </a>
    ...
</div>
<div class="row">
    <div class="col-md-6 preview">
        <form method="post" enctype="multipart/form-data" id='submitForm' action="?type=page&amp;id=4">
            <div id="content_de" class="editor">
                <p>Placeholder Text</p>
            </div>
            <input type='hidden' name='content_de_submission' id='content_de_submission' />
            <div class="text-center">
                <a id="submit_editor" class="btn btn-primary btn-sm" href="#!">Saving Changes</a>
            </div>
        </form>
    </div>
    ...
</div>

bildschirmfoto vom 2015-09-21 12-28-36

Is my HTML or the Javascript wrong

@codewithtyler
Copy link
Contributor Author

@Findus23 thanks for posting your issue with the following fix. I copied what your example into an HTML file and can see the issue you're referring to. Unfortunately, I won't be able to take a closer look at it until this afternoon.

In the meantime, can you test something for me? Can you open each of the example html files in your browser and tell me if those work for you? That will help me with some initial troubleshooting.

Edit: By the way, what's the HTML form for?

@Findus23
Copy link
Contributor

I have tested the examples and they are working correctly, except for form-post.html. (and multiple-editors.html, but this is another issue: #31).
That way I have found out that the problem is Line 92 :

wrapper = $(editor).parent(),

The editor is only looking for toobar buttons in its direct parent. But in my case this is the <form>, which doesn't contain the toolbar.

By the way, what's the HTML form for?
I don't exactly understand, what you mean. I am writing a CMS and sending the content of the editor to the server to process it with PHP.

@codewithtyler
Copy link
Contributor Author

Ah ok. I haven't worked on any implentations of this outside of HTML and ASP.NET, so I wasn't sure.

Would this work with what you're trying to do?

    <body>
        <div class="container">
            <div class="row">
                <div class="col-md-6 preview">
                    <form method="post" enctype="multipart/form-data" id='submitForm' action="?type=page&amp;id=4">
                        <div class="btn-toolbar" data-role="editor-toolbar" data-target="#content_de">
                            <div class="btn-group">
                                <a class="btn btn-default" data-edit="format-p" title="Absätze">
                                    <i class="fa fa-paragraph"></i>
                                </a>
                            </div>
                            <div class="btn-group">
                                <a class="btn btn-default" data-edit="bold" title="Bold (Ctrl/Cmd+B)"><i class="fa fa-bold"></i></a>
                                <a class="btn btn-default" data-edit="italic" title="Italic (Ctrl/Cmd+I)"><i class="fa fa-italic"></i></a>
                                <a class="btn btn-default" data-edit="strikethrough" title="Strikethrough"><i class="fa fa-strikethrough"></i></a>
                                <a class="btn btn-default" data-edit="underline" title="Underline (Ctrl/Cmd+U)"><i class="fa fa-underline"></i></a>
                            </div>
                        </div>
                        <div id="content_de" class="lead" data-placeholder="Placeholder text">
                        </div>
                        <div class="text-center">
                            <a id="submit_editor" class="btn btn-primary btn-sm" href="#!">Saving Changes</a>
                        </div>
                    </form>
                </div>
                ...
            </div>
        </div>
        <script type='text/javascript'>$('#content_de').wysiwyg();</script>
    </body>

I moved the form opening tag up to start before the toolbar. This allows the editor to continue to see the toolbar so it can change the colors of the button. I also moved the placeholder text. The editor utilizes a special data attribute called data-placeholder in order to create the placeholder text.

@Findus23
Copy link
Contributor

Unfortunately this isn't helping me much.
As you can see I am having two texts, but I only add the editor to one of them at a time via PHP to avoid issue #31. So it is easier for me, if I have the toolbar outside of the "row" div.

bildschirmfoto vom 2015-09-22 15-02-47
bildschirmfoto vom 2015-09-22 15-03-50

Would it be possible to add an option to use a custom wrapper class similar to https://github.com/RandomlyKnighted/bootstrap-wysiwyg/blob/master/src/bootstrap-wysiwyg.js#L323
?

Regarding the placeholder text: This was my mistake. I didn't mean a placeholder like the HTML attribute that disappears if you start typing, but rather a prefilled editor.

@codewithtyler
Copy link
Contributor Author

Ah, I misread that earlier. I thought you were dealing with a single editor. So it appears that the issue you're experience is due to the bug mentioned in issue #31. This PR is not mean to fix that bug. If you will propose your wrapper question there I'll be happy to continue our discussion and create a fix for that bug. I believe with a bit more discussion we can come up with a good solution to the problem you are experiencing.

@codewithtyler
Copy link
Contributor Author

You can ignore my previous comment. I got some additional time this afternoon to look over what you mentioned. To put it simply in the following line the editor

$(options.toolbarSelector,wrapper).find(toolbarBtnSelector).each(function () {

searches all elements that matches the toolbarSelector and the wrapper criteria and applies the background color to those elements. I think the original reason this was done was to accommodate situations like yours and to also accommodate the typical situation where the toolbar is a child of the parent element. However, this was not necessary as toolbarSelector handles both situations due to the toolbar having the data-role attribute.

@codewithtyler
Copy link
Contributor Author

My latest commit should fix your issue.

@Findus23
Copy link
Contributor

That's odd. I somehow used commit https://github.com/RandomlyKnighted/bootstrap-wysiwyg/commit/d22e3aee5c and I am now unable to find out why it doesn't appear in the list of commits (https://github.com/RandomlyKnighted/bootstrap-wysiwyg/commits/underscore-fix)

But commit https://github.com/RandomlyKnighted/bootstrap-wysiwyg/commit/1e2f216a91e71e6417600cb88aa03d91d1fc5917 is working perfectly and now only needs to be merged into the main repository.

Thanks for helping me.

@codewithtyler
Copy link
Contributor Author

I merged last 3 commits together as one by rebasing them.

No problem, thanks for catching the issue.

@codewithtyler
Copy link
Contributor Author

@steveathon there may be merge conflicts once you merge my other PRs. I'll be happy to rebase this once they have been merged.

Updated grunt, demo and readme.
@steveathon
Copy link
Owner

Thanks @RandomlyKnighted this one is ready to rebase now. 🙅

@codewithtyler
Copy link
Contributor Author

@steveathon give me a few mins and I'll have it fixed up for ya

@codewithtyler
Copy link
Contributor Author

@steveathon alright fixed haha...I'm getting faster at rebasing now

@codewithtyler
Copy link
Contributor Author

After merging this..are we ready to release?

steveathon added a commit that referenced this pull request Sep 30, 2015
Fixed #58 and Fixed #19 - throttle interfering with UI
@steveathon steveathon merged commit 4eb3380 into steveathon:master Sep 30, 2015
@steveathon
Copy link
Owner

Yep, I believe we are. I'll do a version bump tonight.

@codewithtyler
Copy link
Contributor Author

Sounds good. When do you want to do the roadmap so we have a plan for the project? I'd like to know what our goals are from here.

@codewithtyler codewithtyler deleted the underscore-fix branch September 30, 2015 00:46
@codewithtyler
Copy link
Contributor Author

@steveathon don't forget to release the new version

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.

3 participants