calcite-checkbox #68
calcite-checkbox #68
Conversation
- The test was relying on a guid implementation which used - window.crypto.getRandomValues(), which is not available in Jest - I replaced it with Math.random() - Which is sufficiently random for generating UUIDs - I also refactored the guid function for fun.
| }); | ||
|
|
||
| // Not sure why this is failing; it works in real life | ||
| xit("toggles the checked attributes when the inner checkbox is toggled", async () => { |
There was a problem hiding this comment.
Just want to call out this failing test. The behavior works in "real life" and I can't figure out what else to do here.
| .map(count => { | ||
| let out: string = ""; | ||
| for (let i = 0; i < count; i++) { | ||
| out += (((1 + Math.random()) * 0x10000) | 0).toString(16).substring(1); |
There was a problem hiding this comment.
I edited this utility function because it was causing a test failure: window.crypto.getRandomValues() is not available in Jest, so I replaced it with plain old Math.random(). Math.random() is not as cryptographically secure but that doesn't matter for generating UUIDs; we're concerned here with uniqueness, not randomness.
| } | ||
| } | ||
|
|
||
| @Listen("keydown") keyDownHandler({ keyCode }: KeyboardEvent) { |
| display: none; | ||
| } | ||
|
|
||
| :host { |
There was a problem hiding this comment.
We should probably add the main blue color + hover/disabled as css variables here so that other apps can override with their spot color. Here are a couple examples:
https://github.com/ArcGIS/calcite-components/blob/paulcpederson/slider/src/components/calcite-slider/calcite-slider.scss#L8
https://github.com/ArcGIS/calcite-components/blob/master/src/components/calcite-tabs/calcite-tabs.scss#L4
|
Looks good. Couple comments, nothing major. We should probably be using Sass variables from either calcite-base or calcite-colors for all our color work. That way we'll guarantee all the hex values are the same. I can open a pr with sass color vars later, though. |
|
@julio8a or @macandcheese could probably deliver a spec of which calcite color variables should be used where for this component... |
|
@vcolavin I can work with you on the color variables. |
|
@julio8a great! Here's the comp I was given to match: https://user-images.githubusercontent.com/1031758/58721793-a2b4e380-838a-11e9-8da6-9dde92942d3e.png I'm trying to match the various shades of blue to what is available in calcite-colors. Is there an easy system for matching the colors? |
|
^ We do have a Sketch UI kit. The color styles applied to text and elements are the same as on calcite-base repository variables. I think These are the variables we should be using: These match all styles on Sketch file. |
|
|
C'est fini! This PR addresses the following issues: #1, #67
It only took my first month working here at esri dot com to write this checkbox:
I also fixed the tests.
Tests on
master:Tests on this branch:
