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

Add getters and setters for height and color options #1145

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

tf
Copy link
Contributor

@tf tf commented Jun 20, 2017

Allow updating the waveColor, progressColor, cursorColor and height options that were passed as part of the options for create.

For the wave color options, simply ensure that the wave is redrawn. For the height option, delegate to drawer.setHeight to handle the update. Note that - while drawer applies the pixelRatio itself when initializing from params - setHeight expects a height that already has been multiplied by pixelRatio.

The cursor is a border on one of the wave elements. Add an updateCursor method to the drawer interface to allow updating the cursor related styles.

Redrawing the wave via drawBuffer only works correctly after #1144 has been merged.

@agamemnus
Copy link
Contributor

it('should allow setting waveColor', function () {...

@tf
Copy link
Contributor Author

tf commented Jun 20, 2017

@agamemnus what does that mean?

@agamemnus
Copy link
Contributor

agamemnus commented Jun 20, 2017 via email

@tf
Copy link
Contributor Author

tf commented Jun 20, 2017

I know. What do you want me to change?

@agamemnus
Copy link
Contributor

agamemnus commented Jun 20, 2017 via email

@tf
Copy link
Contributor Author

tf commented Jun 20, 2017

Still not quite sure we are talking about the same thing, but there is a test suite that already covers things like toggling mute, so I also added some tests for the getters and setters that I added.

I don't see it working

The tests run on Travis. The new ones are also passing.

@agamemnus
Copy link
Contributor

Oh, ok (I was on my phone).

@tf tf changed the title Add getters and setters for height and color options [WIP] Add getters and setters for height and color options Jun 20, 2017
@tf
Copy link
Contributor Author

tf commented Jun 20, 2017

I'll also add getters/setters for cursorColor to cover all color options. I'll update the PR tomorrow.

@agamemnus
Copy link
Contributor

I have a different method that I put as a PR. Basically using get and set:

#1132
#1130

@tf tf force-pushed the height-color-get-set branch from b8dd896 to dbaeefe Compare June 21, 2017 09:05
@tf tf changed the title [WIP] Add getters and setters for height and color options Add getters and setters for height and color options Jun 21, 2017
@tf
Copy link
Contributor Author

tf commented Jun 21, 2017

I've updated the PR to also allow updating the cursor color.

@agamemnus From my point of view, the methods added by this PR fit in quite nicely with the existing API and completely cover my use case. So I'll leave this PR like this, as one option. If things are supposed to move in a different direction, though, I'm happy to port my application to a different API once a future release of Wavesurfer provides the features added here.

@tf tf force-pushed the height-color-get-set branch from dbaeefe to ff05330 Compare June 21, 2017 17:28
Copy link
Contributor

@thijstriemstra thijstriemstra left a comment

Choose a reason for hiding this comment

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

Looks good. Can you document these new methods in src/wavesurfer.js?

@tf
Copy link
Contributor Author

tf commented Jul 10, 2017

Sure. Just I'll update the PR this week.

@tf
Copy link
Contributor Author

tf commented Jul 10, 2017

@thijstriemstra Added the inline docs. Tried to follow the conventions found on the next branch.

@tf
Copy link
Contributor Author

tf commented Jul 25, 2017

@thijstriemstra All of a sudden I am seeing a whole bunch of unrelated commits in this and my two other PRs. Shall I recreate the PRs? What happened?

@mspae
Copy link
Contributor

mspae commented Aug 18, 2017

@tf the next (v2) branch was set to be the master branch, and the old master is now es5 (version 1) – you need to rebase your commits onto the new master branch.

@katspaugh
Copy link
Owner

@tf you can also change the base branch from this PR page directly.

@tf tf force-pushed the height-color-get-set branch 2 times, most recently from e92bd64 to 813439d Compare November 3, 2017 16:08
Allow updating the `waveColor`, `progressColor`, `cursorColor` and
`height` options that were passed as part of the options for create.

For the wave color options, simply ensure that the wave is
redrawn. For the height option, delegate to `drawer.setHeight` to
handle the update. Note that - while drawer applies the `pixelRatio`
itself when initializing from `params` - `setHeight` expects a height
that already has been multiplied by `pixelRatio`.

The cursor is a border on one of the wave elements. Add an
`updateCursor` method to the drawer interface to allow updating the
cursor related styles.
@tf tf force-pushed the height-color-get-set branch from 813439d to 05d5fa8 Compare November 3, 2017 16:09
@tf
Copy link
Contributor Author

tf commented Nov 3, 2017

@katspaugh @thijstriemstra rebased and updated.

@thijstriemstra thijstriemstra merged commit e3fb34a into katspaugh:master Nov 3, 2017
@tf tf deleted the height-color-get-set branch November 3, 2017 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants