regions plugin: add editableContent and removeButton options

[ApayRus]

Region plugin param {contentEditable: true} adds an editable div on top of each region.

When editing is finished (onBlur) it fires event:

{ action: 'contentEdited', oldText, text }

You can handle it with:

wavesurfer.on('region-updated', (region, event) => callback)

New text will be saved in region.data.text.

To fill regions with text on initialization, provide them with a property data.text.

The whole region will be looking like:

{
    start: 1, 
    end: 2, 
    data: {
        text: 'some text'
        }
}

Region plugin param removeButton adds a little x to right bottom corner of each region.

Related Issues and other PRs:

fixes #2513

[coveralls]

Coverage increased (+0.07%) to 81.375% when pulling e2b94f934dba2eb69bd8fbb7cdeef5cf5478eca2 on ApayRus:master into 61005bcd79c7aa3c6c61cb10488d75321c27d81c on katspaugh:master.

[msuhov]

@ApayRus @thijstriemstra Hi all! I don't think both things (editableContent and removeButton) should be done in the library's repo, as it can be easily done only when it's needed in the project itself, with project's styles, classnames and etc.: Basically you can use region.element to trick the Region's DOM element

const region = wavesurfer.addRegion({
        start: 1,
        end: 3,
        loop: false,
        color: 'hsla(400, 100%, 30%, 0.5)'
    });

addRegionRemoveButton(region);

// Add region remove button
function addRegionRemoveButton(region) {
    const removeButtonEl = document.createElement('div');
    removeButtonEl.className = 'remove-region-button';
    removeButtonEl.innerText = 'x';
    region.element.appendChild(removeButtonEl);
    const css = {
        zIndex: 4,
        position: 'absolute',
        bottom: 0,
        right: 0,
        cursor:'pointer',
        marginRight: '3px',
        fontSize: '80%',
        color: 'grey',
        height: '14px'
    };
    region.style(removeButtonEl, css);

    // TODO: Add event listeners to an upper-scope array to remove listeners after region destroy
    removeButtonEl.addEventListener('click', (event) => {
        event.stopPropagation();
        this.remove();
    });
}

[ApayRus]

thoughts @ApayRus?

I like my solution. It is easy to implement with to strings (I already use it in a project, from my fork). What Max said takes a lot of work-around code and logic. If he doesn't need this functionality, he may not use it.

@vitalii-bulyzhyn what do you think?

[vitalii-bulyzhyn]

I didn't look through the code, however from the perspective of the person who is working with Region plugin - this is really logical to be able to pass text into the region directly and have a delete button there

[msuhov]

@thijstriemstra Thanks for your answer My only concern is about to specific things like styles and textContent of a remove button. Styles, like grey colour or margin of 3px, shouldn't be in the repo from my point of view, because it means that client apps will have a bunch of !important overrides. The same goes for textContent="x" - most of real-world project may want to have an icon element there or something like that. So at the minute remove button is not quite configurable.

Would it be better to add a render function to a region to add/remove some DOM elements, so we don't have to 'trick' region.element?

[thijstriemstra]

Styles, like grey colour or margin of 3px, shouldn't be in the repo from my point of view, because it means that client apps will have a bunch of !important overrides.

I agree, but a class name was added that allows you to override these things. I agree the css should be as minimal as possible, @ApayRus can you address @msuhov's concerns?

[rbracco]

Hey all, I was asked to try and comment on this PR by @thijstriemstra in regards to my issue #2531. I've found that to override anything in .remove-region-button, I need to use !important in css which to me is a code smell.

Overall, I agree in principle with @msuhov about these features not being part of the core repo, I feel like the reality is there are lots of people using WaveSurfer who just want to build working stuff quickly and don't necessarily have the knowledge to implement, so I would lean towards including this functionality out of the box. I do agree that using the 'x' character as the button is problematic as most will want an icon and I can't figure out how to override that from CSS.

My recommendations are to...

1. Style the initial css in such a way that !important isn't needed to make basic style changes.
2. Make the content of the button easily editable (maybe this is possible in CSS already and I just don't know how)
3. Change the region-remove-button from a div to a button for accessibility reasons. You can reset the button style with

padding: 0;
background: none;
border: none;

and it will look exactly how it looks now but it will be reachable without the mouse.

[ApayRus]

@thijstriemstra : @ApayRus can you address @msuhov's concerns?

I hope yes. But I need more time. I am busy these days.

@rbracco, thank you for your feedback.

[thijstriemstra]

@ApayRus I'd like to include this in the next release, any news?

[ApayRus]

@ApayRus I'd like to include this in the next release, any news?

Sorry I haven't done all these things above. If there is a time until new release, I will try to improve code. When you plan to release?

[thijstriemstra]

When you plan to release?

it can wait; a week or something, is that enough?

[ApayRus]

When you plan to release?

it can wait; a week or something, is that enough?

I hope yes :)

[thijstriemstra]

any news @ApayRus?

[thijstriemstra]

@katspaugh I'm surprised you merged this and added it to the changelog for an already released 6.3.0, can you explain why?

[katspaugh]

Because that's the problem with a manually updated changelog – it needs to be up-to-date (in this case it wasn't) and it creates git conflicts.