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

[feature]: add option to send a default waveform array #166

Open
DanielFRico opened this issue Feb 21, 2025 · 4 comments
Open

[feature]: add option to send a default waveform array #166

DanielFRico opened this issue Feb 21, 2025 · 4 comments
Labels
enhancement New feature or request

Comments

@DanielFRico
Copy link
Contributor

Hi team! In our project in order to make wave forms build quickly and displayed fast to the user We already have an array of waveform of the audio (array previously built using extractWaveformData function). Currently, this option is not available in the library and I made local patch to support this feature and send array as argument to the component but I have notice that WaveForm component has a important complexity, life cycle methods, events and so on. So I would to suggest you this important feature because I think that a faster way to display component to the user is passing a default array of waveform. Now, tell me what do you think about that. I think would be a good new feature that will speed up build component time!

@kuldip-simform
Copy link
Contributor

@DanielFRico Thank you for your valuable suggestion! Our current focus is on computing waveforms dynamically rather than relying on precomputed data. However, we are always open to contributions that enhance the user experience.

If you could submit a PR with your implementation, we’d be happy to review it, test edge cases, and explore how it fits into the library. Looking forward to your contribution!

@kuldip-simform kuldip-simform added the enhancement New feature or request label Feb 24, 2025
@DanielFRico
Copy link
Contributor Author

DanielFRico commented Feb 25, 2025

@DanielFRico Thank you for your valuable suggestion! Our current focus is on computing waveforms dynamically rather than relying on precomputed data. However, we are always open to contributions that enhance the user experience.

If you could submit a PR with your implementation, we’d be happy to review it, test edge cases, and explore how it fits into the library. Looking forward to your contribution!

Hi @kuldip-simform Nice to talk to you. About this enhancement I have done a local patch like a workaround. But, what I can see is that the number of the examples is related to the available screen width and also depends of candleWidth and candleSpace. Now if I set a default waveform array this applied logic is going to conflict with those properties. Because I can for instance, send 20 values and screen is set for 30 or send 40 values. So, to solve this situation I ignored candleWidth and candleSpace values when rendering every WaveformCandle. Instead of that, I used % to set a dynamic width to every candle. For instance, if I send 10 values every WaveformCandle will have a 10% of width of its container making possible to print in a correct way those values. So, for that reason this worked for our particular situation but if we want to make a good approach we need to give a better solution. And as you have a bigger context I would to know what do you think about this enhancement. Let me know it. Ty!

I think to crate a less complex component and exported for this particular situation could be a good approach. Because if a waveform is already set we need less code or calculations. Only will matter player events (start, stop, resume) and a WaveformCandle that adjust using % according to elements size instead of taking into account candleWidth and candleSpace

@kuldip-simform
Copy link
Contributor

@DanielFRico
This approach is interesting, but dynamically adjusting the width based on percentages might lead to inconsistent visuals, especially if candle widths vary significantly. Additionally, since we already provide customization options for candle width and height, this approach could make it harder to maintain that flexibility.

We’ll discuss this internally with the team and get back to you with our thoughts.

That being said, we’re particularly interested in understanding how you store previously calculated waveform data. If it’s not already calculated, how do you handle that scenario? This insight would help us evaluate the best approach moving forward.

@DanielFRico
Copy link
Contributor Author

DanielFRico commented Feb 28, 2025

@DanielFRico This approach is interesting, but dynamically adjusting the width based on percentages might lead to inconsistent visuals, especially if candle widths vary significantly. Additionally, since we already provide customization options for candle width and height, this approach could make it harder to maintain that flexibility.

We’ll discuss this internally with the team and get back to you with our thoughts.

That being said, we’re particularly interested in understanding how you store previously calculated waveform data. If it’s not already calculated, how do you handle that scenario? This insight would help us evaluate the best approach moving forward.

@kuldip-simform of course. The current code logic and implementation makes this refactor a bit complicated in terms to adapt current code with this feature. But, for that reason if in a future you decide to implement it I recommend to create a new Waveform component that reuse common code between this new one and existing Waveform to share common logic (could be a hook with common functionalities) and implement separately to have a component as the current and new one that can receive an array of waveform. It's my suggestion. Have a nice day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants