Skip to content

Set/change pen color blocks with color, brightness, saturation, transparency #725

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

Merged

Conversation

ericrosenbaum
Copy link
Contributor

@ericrosenbaum ericrosenbaum commented Oct 18, 2017

Resolves

Resolves scratchfoundation/scratch-blocks#1091
Resolves scratchfoundation/scratch-gui#783

And progress on scratchfoundation/scratch-gui#725

Note that testing these changes will require pulling a PR on Scratch-gui that updates the extension library modal: scratchfoundation/scratch-gui#793

Proposed Changes

Change the way the pen color is set to use a menu of four color parameters: color, brightness, saturation and transparency. All are scaled 0-100. Color is wrapped, the rest are clamped.

Reason for Changes

The earlier version of pen color blocks used different blocks with different ranges for the color parameters (and the "shade" block combined saturation and brightness). This change simplifies the color parameters by putting them all into the same range. It also reduces the total number of pen blocks by putting the four parameters into a menu.

@paulkaplan
Copy link
Contributor

Lint is failing

/home/travis/build/LLK/scratch-vm/src/blocks/scratch3_pen.js
  324:1   error  Expected indentation of 24 spaces but found 20  indent
  415:13  error  'rgb' is never reassigned. Use 'const' instead  prefer-const
  436:1   error  Expected indentation of 8 spaces but found 12   indent
  437:1   error  Expected indentation of 12 spaces but found 16  indent
  438:1   error  Expected indentation of 12 spaces but found 16  indent
  439:1   error  Expected indentation of 8 spaces but found 12   indent
  440:1   error  Expected indentation of 12 spaces but found 16  indent
  441:1   error  Expected indentation of 12 spaces but found 16  indent
  442:1   error  Expected indentation of 8 spaces but found 12   indent
  443:1   error  Expected indentation of 12 spaces but found 16  indent
  444:1   error  Expected indentation of 12 spaces but found 16  indent
  445:1   error  Expected indentation of 8 spaces but found 12   indent
  446:1   error  Expected indentation of 12 spaces but found 16  indent
  447:1   error  Expected indentation of 12 spaces but found 16  indent
  448:1   error  Expected indentation of 8 spaces but found 12   indent
  449:1   error  Expected indentation of 12 spaces but found 16  indent

@ericrosenbaum
Copy link
Contributor Author

Note that we are hoping to do a workshop on Wednesday Oct 25th using this version on develop, so lets try to merge and deploy by then if possible

Copy link
Contributor

@paulkaplan paulkaplan left a comment

Choose a reason for hiding this comment

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

Code looks good to me, and confirmed it plays nice with GUI (that PR mentioned in the description is already merged).

@cwillisf I think it is probably important for you to take a look at this also though.

@chrisgarrity
Copy link
Contributor

@cwillisf please /cc me when this is ready so I know if we can use it on Wednesday for the workshop. Thanks!

Copy link
Contributor

@thisandagain thisandagain left a comment

Choose a reason for hiding this comment

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

This looks good to me and is working well locally. Thanks @ericrosenbaum

@thisandagain thisandagain removed their assignment Oct 23, 2017
@ericrosenbaum ericrosenbaum merged commit 9ffbd60 into scratchfoundation:develop Oct 23, 2017
@ericrosenbaum ericrosenbaum deleted the feature/pen-extension branch October 23, 2017 15:25
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

The code itself looks good (with one minor comment on naming or documentation) but I'm concerned about compatibility. It seems like this will break compatibility with pen projects from Scratch 2.0, especially with the change to the wrapping behavior. Is there a plan for that?

* @returns {number} the clamped value.
* @private
*/
_clampTransparency (value) {
_clampColorParam (value) {

This comment was marked as abuse.

This comment was marked as abuse.

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