feat(checkbox): update component specs #846

100stacks · 2021-03-11T07:55:59Z

Description

Update Checkbox specs.

What are the relevant story cards/tickets? Any additional PRs or other references?

Jira: SURF-2182

Before you request a review for this PR:

For UI changes, did you manually test in recent versions of modern browsers (Chrome, Firefox, and Safari)?
For UI changes, did you manually test in IE11 and legacy Edge?
Did you add component tests for any new code?
Did you run the component unit tests via yarn test to ensure all tests passed?
Did you include a screenshot of the component tests?
If you changed/added functionality, did you update the demo page and documentation?
If needed, did you add or modify the demo test page to test the changed/added functionality?
Did you assign reviewers?
In Jira, have you linked to this PR on the ticket(s)?

netlify · 2021-03-11T07:56:04Z

Deploy preview for helix-ui ready!

Built with commit 2538b24

https://deploy-preview-846--helix-ui.netlify.app

badejayayesubabu

DEV LGTM

ssalinas24

@100stacks been a while since I looked at these inner helix component files. So I had some questions around understanding some of the redundant fallback properties. I think I also found a couple of small changes. We can zoom over these changes. It would probably help me start catching up on understanding how far all of this code has evolved. thanks.

ssalinas24 · 2021-03-11T16:59:59Z

src/scss/components/checkbox/_index.scss

  background-color: var(--hxCheckbox-bgcolor, $gray-0);
-  border-radius: 2px;
+  border: $gray-500;
  border: 1px solid var(--hxCheckbox-border, $gray-500);


This is good here.
However, when it is in a checked state, the gray border is still active from this block.
Hard to see, but I believe design wants us to have the blue borders around it.

Good catch. I'll fix this.

ssalinas24 · 2021-03-11T17:06:35Z

src/scss/components/checkbox/mixins/_hxCheckboxFacade.scss

    box-shadow: $focus-glow-invalid;
  }
  @else if $pseudo-state == hover {
+    background-color: $red-status-100;
    background-color: var(--hxCheckbox-invalid-hover-bgcolor, $red-status-100);


For the Required state. I agree that this is a rare use case, but is certainly possible in our BPiT forms within CTUI.
Double check with design and confirm the specs with them for the hover background color and the entire state in general. I assume it would just stay the same, but they would need to include this state in their documentation. Seems like it was missed in the design.

@danielmdesigns, can you provide design recommendations for required and invalid states. These states could occur during validation phase (for example, form validation).

ssalinas24 · 2021-03-11T17:09:25Z

src/scss/_vars.scss

@@ -153,7 +153,7 @@ $font-monospace: "Open Sans Condensed", "Roboto Mono", monospace;
 // ==== FOCUS STATE =====
 $focus-glow: 0 0 4px rgba($blue-700, 0.5);
 $focus-glow-disabled: $focus-glow;
-$focus-glow-invalid: 0 0 4px rgba($red-status-900, 0.5);
+$focus-glow-invalid: 0 0 4px rgba($red-status-500, 0.5);


This looks good to me. However, just make sure design includes this in their docs.
The focus states were left out of the checkbox states and missed. I think they were missed for radios and some other components as well.

@danielmdesigns, can you provide design recommendations for required and invalid states. These states could occur during validation phase (for example, form validation).

@ssalinas24 @danielmdesigns we received confirmation from design to keep required and invalid states. I updated these states to match our v2.0 Checkbox component styled specifications.

ssalinas24 · 2021-03-11T17:13:51Z

src/scss/components/checkbox/_index.scss

  background-color: var(--hxCheckbox-bgcolor, $gray-0);
-  border-radius: 2px;
+  border: $gray-500;


this line is a border short hand with just the color var. The line 86 below it with $gray-500 color already establishes a color var at the end of that line and size. Is line 86 in parenthisis conditional to have CSS VAR || Color Var? If so then I guess you wouldn't need this line 85. If not, then this line 85 would be same as line 86 but with only the color var, right? and line 86 would just have the CSS var, correct?

Good catch. I'll verify which property is correct and update.

ssalinas24 · 2021-03-11T17:17:07Z

src/scss/components/checkbox/_index.scss

      color: var(--hxCheckbox-indeterminate-color, $blue-700);

      ~ label > hx-checkbox {
+        background-color: $blue-700;


@100stacks so i'm just asking in order to gain knowledge. I see we sometimes duplicate properties. So in this case we have the color variable here, and then the next line we have the same property, but with the CSS variable. This is required for browser compatibility and fallback, correct? So if the browser doesn't support CSS variables, then it will at least have the usual color var, correct?

@ssalinas24, yes that's correct. We're using duplicate properties for IE11 support fallback. It's cleaner and simpler than using a library to provide polyfills for IE11 CSS variables/custom property support.

ssalinas24 · 2021-03-11T17:22:33Z

src/scss/components/checkbox/_index.scss

      }
    }

    &:valid {
      ~ label > hx-checkbox:hover {
-        @include hxCheckboxFacade($pseudo-state: hover);
+        @include hxCheckboxFacade($pseudo-state: null);  // v2.0 specs


we didn't still need the hover here? I guess because hover technically is supposed to look the same as the checked state, correct?

Yes, we need clarification from design on hover while the checkbox is checked/selected state.

@danielmdesigns, can you please provide design recommendations for hover state on a checked/selected checkbox.

@ssalinas24, I'm leaving the hover state construct in place for now.

ssalinas24 · 2021-03-11T17:24:37Z

src/scss/components/checkbox/mixins/_hxCheckboxControl.scss


 // ========== PRISTINE ==========
 @mixin __checkboxControl($pseudo-state: null) {
  @if $pseudo-state == focus {
-    box-shadow: $focus-glow;
+    // box-shadow: $focus-glow; // TBD v2.0 specs


So has designed confirmed that we are just getting rid of the box-shadow for all focus states on every element?
If we have them on some elements, but not others, then that is inconsistent. Let me know if they have a made a decision.

ssalinas24 · 2021-03-11T17:28:57Z

src/scss/components/checkbox/mixins/_hxCheckboxFacade.scss

    border-color: var(--hxCheckbox-bordercolor-focus, $blue-900);
-    box-shadow: $focus-glow;
+    // box-shadow: $focus-glow; // TBD v2.0 specs


again let me know if we have a straight answer around box shadows across the design system

Yes, I'll check with @danielmdesigns on his recommendations for checkbox.

ssalinas24 · 2021-03-11T17:30:23Z

src/scss/components/checkbox/mixins/_hxCheckboxControl.scss

@@ -38,21 +43,24 @@
    box-shadow: $focus-glow-invalid;

    &::-ms-check {
-      border-color: var(--hxCheckbox-invalid-bordercolor, $red-status-900);
+      border-color: $red-status-500;
+      border-color: var(--hxCheckbox-invalid-bordercolor, $red-status-500);


similar setup here. Let me know if this is correct, and I'm just wrong in my prior comment in first file.

That's right. This is our pattern to support IE11 fallbacks.

ssalinas24 · 2021-03-11T17:31:03Z

src/scss/components/checkbox/mixins/_hxCheckboxControl.scss

      border-color: var(--hxCheckbox-disabled-bordercolor, $gray-500);
-      box-shadow: $focus-glow-disabled;
+      // box-shadow: $focus-glow-disabled; // TBD v2.0 specs


another box shadow area to confirm

Yes, we need clarification from design on box shadow for checkbox.

100stacks

@ssalinas24, please see PR #846 updates.

100stacks · 2021-03-12T20:03:10Z

src/scss/components/checkbox/_index.scss

      }
    }

    &:valid {
      ~ label > hx-checkbox:hover {
-        @include hxCheckboxFacade($pseudo-state: hover);
+        @include hxCheckboxFacade($pseudo-state: null);  // v2.0 specs


@ssalinas24, I'm leaving the hover state construct in place for now.

100stacks · 2021-03-12T20:06:49Z

src/scss/_vars.scss

@@ -153,7 +153,7 @@ $font-monospace: "Open Sans Condensed", "Roboto Mono", monospace;
 // ==== FOCUS STATE =====
 $focus-glow: 0 0 4px rgba($blue-700, 0.5);
 $focus-glow-disabled: $focus-glow;
-$focus-glow-invalid: 0 0 4px rgba($red-status-900, 0.5);
+$focus-glow-invalid: 0 0 4px rgba($red-status-500, 0.5);


@ssalinas24 @danielmdesigns we received confirmation from design to keep required and invalid states. I updated these states to match our v2.0 Checkbox component styled specifications.

ssalinas24

LGTM

100stacks added CSS 🆕 New Component Specs Updated Component Specifications labels Mar 11, 2021

100stacks added this to the HelixUI-v2.2.0 milestone Mar 11, 2021

100stacks requested review from lalithkarikelli and badejayayesubabu March 11, 2021 07:55

100stacks added Need: Dev LGTM and removed CSS labels Mar 11, 2021

badejayayesubabu approved these changes Mar 11, 2021

View reviewed changes

ssalinas24 suggested changes Mar 11, 2021

View reviewed changes

100stacks requested a review from danielmdesigns March 11, 2021 19:03

100stacks force-pushed the surf-2182-checkbox-specs branch from 76a7623 to d27e3f8 Compare March 11, 2021 19:30

feat(checkbox): update component specs

2538b24

100stacks force-pushed the surf-2182-checkbox-specs branch from d27e3f8 to 2538b24 Compare March 12, 2021 19:58

100stacks commented Mar 12, 2021

View reviewed changes

ssalinas24 approved these changes Mar 15, 2021

View reviewed changes

100stacks removed the request for review from lalithkarikelli March 15, 2021 21:05

100stacks removed the Need: Dev LGTM label Mar 15, 2021

100stacks merged commit f212cce into master Mar 15, 2021

100stacks deleted the surf-2182-checkbox-specs branch March 25, 2021 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(checkbox): update component specs #846

feat(checkbox): update component specs #846

100stacks commented Mar 11, 2021

netlify bot commented Mar 11, 2021 •

edited

Loading

badejayayesubabu left a comment

ssalinas24 left a comment

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

100stacks Mar 12, 2021

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021 •

edited

Loading

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

100stacks Mar 11, 2021

100stacks Mar 12, 2021

ssalinas24 Mar 11, 2021

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

ssalinas24 Mar 11, 2021

100stacks Mar 11, 2021

100stacks left a comment

100stacks Mar 12, 2021

100stacks Mar 12, 2021

ssalinas24 left a comment

feat(checkbox): update component specs #846

feat(checkbox): update component specs #846

Conversation

100stacks commented Mar 11, 2021

Description

What are the relevant story cards/tickets? Any additional PRs or other references?

Before you request a review for this PR:

netlify bot commented Mar 11, 2021 • edited Loading

badejayayesubabu left a comment

Choose a reason for hiding this comment

ssalinas24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100stacks Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100stacks left a comment

Choose a reason for hiding this comment

@ssalinas24, please see PR #846 updates.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssalinas24 left a comment

Choose a reason for hiding this comment

netlify bot commented Mar 11, 2021 •

edited

Loading

100stacks Mar 11, 2021 •

edited

Loading