GetmeUK / ContentTools

A JS library for building WYSIWYG editors for HTML content.
http://getcontenttools.com
MIT License
3.95k stars 395 forks source link

Bug in highlightRegions? #443

Closed capr closed 6 years ago

capr commented 6 years ago

I'm trying to highlight all the regions for half a second when starting editing.

For this I do

    editor.addEventListener('start', function(ev) {
        editor.highlightRegions(true)
        setTimeout(function() {
            editor.highlightRegions(false) 
        }, 500)
    })

Hiding the highlighting on a timer works with regions but fixtures remain highlighted. Is this a bug?

My workaround for this is ugly:


    editor.addEventListener('start', function(ev) {
        $('[data-editable], [data-fixture]').addClass('ct--highlight')
        setTimeout(function() {
            $('.ct--highlight').removeClass('ct--highlight')
        }, 500)
    })

    editor.addEventListener('stop', function(ev) {
        setTimeout(function() {
            $('.ct--highlight').removeClass('ct--highlight')
        }, 0)
    })
anthonyjb commented 6 years ago

Hi @capr - so it's not a bug it's because the start event is triggered before the regions are selected and therefore the highlightRegions function has no DOM regions to add/remove the class from. Ideally we could do with perhaps adding a started and stopped event you can bind to (will add in the next release).

In the mean time I'd suggest that you can resolve the issue like so (not ideal but at least slightly shorter):

editor.addEventListener('start', function(ev) {
    setTimeout(function() { editor.highlightRegions(true) }, 0)
    setTimeout(function() { editor.highlightRegions(false) }, 500)
}
capr commented 6 years ago

Thanks for the response and for considering adding those events.

The reason I add/remove the highlight class by hand is because highlightRegions() doesn't also highlight the fixtures.

anthonyjb commented 6 years ago

@capr thanks for reporting this this actually turned out to be a far more significant issue with fixtures than it appears on the surface. The latest release of CT moved to replacing image fixtures over simply replacing their content, this meant that each - init, save or revert - new DOM elements were created for each fixture but the editor wasn't keeping track of this change and so the _domRegions property lost the references to fixtures. This didn't effect editing when a selector is used because the regions/fixtures are reselected each time, but did effect editing if you passed one or more fixture elements to the editor on init. So good spot!

anthonyjb commented 6 years ago

P.S I also added started and stopped events to the editor, so no need for the setTimeout(..., 0) hack above any longer, this should now work:

editor.addEventListener('started', function(ev) {
    editor.highlightRegions(true)
    setTimeout(function() { editor.highlightRegions(false) }, 500)
}