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

Print a warning if block JSON color value is invalid #1639

Merged
merged 1 commit into from
Feb 21, 2018

Conversation

AnmAtAnm
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

#1633: If block definition has invalid color, creating/loading fails/throws

Proposed Changes

Catch the throw error, and print a warning with the block type name.
Without a color assignment, the block defaults to black (same as not having a color property in the JSON).

Reason for Changes

While it is still illegal to have a bad color value specified in the JSON, the consequences of the thrown error outweighed the functional error. Instead catch the error, allow the block to render. This also allows the the remainder of the toolbox/workspace to load.

Test Coverage

This is covered in the new playground test blocks, and the "Styles" category fully loads with this change, with the following warning output in the console:

block.js:1072 Block "block_colour_hex4": Illegal color value:  #992aff99
block.js:1072 Block "block_colour_hex5": Illegal color value:  #NotHex

Tested on:

  • Desktop Chrome

Ignores the color, defaulting to black.
Copy link
Contributor

@picklesrus picklesrus left a comment

Choose a reason for hiding this comment

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

lgtm

@AnmAtAnm AnmAtAnm merged commit 9cd6b1e into google:develop Feb 21, 2018
@AnmAtAnm AnmAtAnm deleted the bad-color-parsing branch February 21, 2018 22:12
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