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.

Sign up for GitHub

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

Feat: [Regions] contenteditable content #3453

Merged
merged 5 commits into from
Jan 4, 2024
Merged

Feat: [Regions] contenteditable content #3453

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5320a23
feat: [regions] add contenteditable property to content
kakashy Dec 27, 2023
7168d42
Merge branch 'katspaugh:main' into main
kakashy Dec 27, 2023
fa51ae2
fix: send update event after content has been edited
kakashy Dec 29, 2023
cf7acd3
Merge branch 'main' of github.com:kakashy/wavesurfer.js into main
kakashy Dec 29, 2023
cab9ccc
chore: updated with upstream changes & fix: review notes
kakashy Jan 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/plugins/regions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
0
Copy link
Owner

Choose a reason for hiding this comment

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

Nani?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no! That's embarrasing :D

/**
* Regions are visual overlays on the waveform that can be used to mark segments of audio.
* Regions can be clicked on, dragged and resized.
Expand Down Expand Up @@ -60,6 +61,8 @@ export type RegionParams = {
maxLength?: number
/** The index of the channel */
channelIdx?: number
/** Allow/Disallow contenteditable property for content */
contentEditable?: boolean
}

class SingleRegion extends EventEmitter<RegionEvents> {
Expand All @@ -74,6 +77,7 @@ class SingleRegion extends EventEmitter<RegionEvents> {
public minLength = 0
public maxLength = Infinity
public channelIdx: number
public contentEditable = false

constructor(params: RegionParams, private totalDuration: number, private numberOfChannels = 0) {
super()
Expand All @@ -87,6 +91,7 @@ class SingleRegion extends EventEmitter<RegionEvents> {
this.minLength = params.minLength ?? this.minLength
this.maxLength = params.maxLength ?? this.maxLength
this.channelIdx = params.channelIdx ?? -1
this.contentEditable = params.contentEditable ?? this.contentEditable
this.element = this.initElement()
this.setContent(params.content)
this.setPart()
Expand Down Expand Up @@ -234,6 +239,13 @@ class SingleRegion extends EventEmitter<RegionEvents> {
this.drag && this.emit('update-end')
},
)

if (this.contentEditable && this.content) {
this.content.addEventListener('click', (e) => this.onContentClick(e))
}
if (this.contentEditable && this.content) {
this.content.addEventListener('blur', (e) => this.onContentBlur(e))
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if (this.contentEditable && this.content) {
this.content.addEventListener('click', (e) => this.onContentClick(e))
}
if (this.contentEditable && this.content) {
this.content.addEventListener('blur', (e) => this.onContentBlur(e))
}
if (this.contentEditable && this.content) {
this.content.addEventListener('click', (e) => this.onContentClick(e))
this.content.addEventListener('blur', (e) => this.onContentBlur(e))
}

}

public _onUpdate(dx: number, side?: 'start' | 'end') {
Expand Down Expand Up @@ -274,6 +286,16 @@ class SingleRegion extends EventEmitter<RegionEvents> {
this.emit('update-end')
}

private onContentClick(event: MouseEvent) {
event.stopPropagation()
const contentContainer = event.target as HTMLDivElement
contentContainer.focus()
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this necessary btw? Isn't focusing the default action on click on editable elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but with the region container having an event listener for clicks, this (focus on text content) does not happen. The event is immediately propagated to its parent. This function is in hopes that we can step in and handle the content property before propagating the event further. In the latest commit, I have made changes to emit the region-clicked event:

private onContentClick(event: MouseEvent) {
    event.stopPropagation()
    const contentContainer = event.target as HTMLDivElement
    contentContainer.focus()
    this.emit('click', event)
  }

}

public onContentBlur(event: FocusEvent) {
this.emit('update-end')
}

public _setTotalDuration(totalDuration: number) {
this.totalDuration = totalDuration
this.renderPosition()
Expand All @@ -297,6 +319,9 @@ class SingleRegion extends EventEmitter<RegionEvents> {
this.content.style.padding = `0.2em ${isMarker ? 0.2 : 0.4}em`
this.content.style.display = 'inline-block'
this.content.textContent = content
if (this.contentEditable) {
this.content.contentEditable = 'true'
}
} else {
this.content = content
}
Expand Down