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

Feat: add part attribute to timeline #3481

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

brian-byassee
Copy link
Contributor

Short description

Add part attributes to the other remaining elements rendered in the wavesurfer

Resolves #
Makes writing css easier outside the shadow root

Implementation details

  • timeline
  • canvases
  • canvas
  • canvas-wave
  • canvas-progress
  • canvas-container
  • progress-container

How to test it

Screenshots

image

Checklist

  • This PR is covered by e2e tests
  • It introduces no breaking API changes

Copy link
Owner

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Thanks!
Could you describe the use case for each of the selectors? Timeline is clear but the rest of them were intentionally not included because messing with their CSS can break wavesurfer. If not now then potentially in some future update where we might decide to change the inner HTML structure.
Adding a part attribute makes an element a part of the external API. So I would consider it only if you have a good use case in mind. Cheers!

@brian-byassee
Copy link
Contributor Author

Completely understand and maybe our approach isn't correct which is why we are having to jump through some hoops.

We're using WaveSurfer to display a waveform with additional content before and after the canvases and progress, within the wrapper. Initially, elements like the timeline-wrapper and our custom transcript-wrapper appeared over the waveform, which caused overlap issues with audio spikes.

image

To accommodate these elements, we've been using JavaScript to adjust the CSS post-render. This approach, however, leads to an initial render followed by a repaint once our styles are applied. We're looking for a solution that allows CSS styling directly, avoiding this two-step rendering process.

This is our current hacked in solution because without the parts we can't use css selectors

querySelector

  instance.getWrapper().style.height = `${PLAYER_HEIGHT}px`;
  instance.getWrapper().querySelector(':scope [part=timeline-wrapper] > div').style.top = `${TRIANGLE_HEIGHT}px`;
  instance.getWrapper().querySelector(':scope .canvases').style.minHeight = `${WAVE_HEIGHT}px`;
  instance.getWrapper().querySelector(':scope .progress').style.minHeight = `${WAVE_HEIGHT}px`;
  instance.getWrapper().querySelector(':scope .canvases > div').style.height = `${
    TIMELINE_HEIGHT + GAP_HEIGHT + WAVE_HEIGHT + GAP_HEIGHT
  }px`;

html
image

@katspaugh
Copy link
Owner

katspaugh commented Jan 11, 2024

Initially, elements like the timeline-wrapper and our custom transcript-wrapper appeared over the waveform, which caused overlap issues with audio spikes.

If you just want to increase spacing at the top and bottom of the waveform, you can simply set a fractional barHeight, e.g. 0.6 and a bigger height.
https://wavesurfer.xyz/examples/?609afb7f9ca8a11e40ea9eb0fea9b4e2

E.g. if you desired waveform height is 100px and you want to have 10 empty pixels above and below:

WaveSurfer.create({
  height: 120,
  barHeight: 0.8, // stretch only to 80% of the height
})

I realize it's not the most obvious way to do this, perhaps we could add a "padding" option.

@brian-byassee
Copy link
Contributor Author

We would like timeline and transcript plugins on their own row so that regions and waveforms will never overlap them.

image

@katspaugh
Copy link
Owner

I see. Yes, in the current setup all plugins are dumped into the same wrapper div, and I can see how this can be problematic.

Regions in particular stretch to 100% of the wrapper, and AFAIR some users did prefer that. So perhaps you could reduce the height of Regions via CSS, as they do have a part selector.

I'll think about plugin isolation a bit more, perhaps there's a neat way to stack them instead of overlapping even if they use position: absolute like Regions.

@brian-byassee
Copy link
Contributor Author

@katspaugh Any thoughts or updates on this PR?

@katspaugh
Copy link
Owner

I suggest that we keep the timeline part but not add the other ones as they are internal elements, not public API.

@katspaugh
Copy link
Owner

@brian-byassee btw, I've invited you to be a collaborator in this repository, so that it's easier for you to make PRs. You can now push directly to this repo instead of a fork. Cheers!

@brian-byassee
Copy link
Contributor Author

@katspaugh I noticed and thank you! 🎊

@brian-byassee brian-byassee changed the title Feat: add part attributes Feat: add part attribute to timeline Jan 24, 2024
Copy link
Owner

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

👍

@katspaugh katspaugh merged commit aa4c3df into katspaugh:main Jan 25, 2024
2 checks passed
@brian-byassee brian-byassee deleted the feat-part-attributes branch January 25, 2024 21:27
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