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.

Sign up for GitHub

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

Switch component for charting basic component #24

Closed
wants to merge 15 commits into from
Closed

Switch component for charting basic component #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2a9d597
Added basic switch component
kumarGayu May 24, 2019
d73218a
Written test cases for calcite-switch
kumarGayu May 24, 2019
6e2beb7
corrected unwanted test cases
kumarGayu May 24, 2019
b1d02b7
Added calcite-switch with min test cases working
kumarGayu May 24, 2019
ab30547
Added destructive and sample on indec.html
kumarGayu May 24, 2019
bc83ede
Added slot and customer can read input
kumarGayu May 25, 2019
390c6a3
Added BEM and updated test cases.
kumarGayu May 25, 2019
f25af5e
emitting the change if no input from slot.
kumarGayu May 25, 2019
7064cfd
formatted the document.
kumarGayu May 25, 2019
cc22059
Dispatch change event on a slotted input inside the label.
kumarGayu May 28, 2019
6709244
Updated stencil config to include the calcite-switch
kumarGayu May 28, 2019
6cbc398
Updated Html to show switches as global navigation
kumarGayu May 28, 2019
3446c89
keypress event on switch box.
kumarGayu May 29, 2019
686f36c
Added latest plan of moving this.
kumarGayu Jun 3, 2019
d8540bc
sample calcite-switch
kumarGayu Jun 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/components.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ export namespace Components {
*/
'middle': string;
}
interface CalciteSwitch {
'checked': boolean;
'color': "red" | "green";
'disabled': boolean;
'focus': any;
'name': string;
'value': string | number | string[];
}
interface CalciteTab {
'getTabIndex': () => Promise<any>;
'id': string;
Expand Down Expand Up @@ -110,6 +118,15 @@ declare namespace LocalJSX {
*/
'middle'?: string;
}
interface CalciteSwitch extends JSXBase.HTMLAttributes {
'checked'?: boolean;
'color'?: "red" | "green";
'disabled'?: boolean;
'focus'?: any;
'name'?: string;
'onSwitchChange'?: (event: CustomEvent<any>) => void;
'value'?: string | number | string[];
}
interface CalciteTab extends JSXBase.HTMLAttributes {
'id'?: string;
'isActive'?: boolean;
Expand Down Expand Up @@ -142,6 +159,7 @@ declare namespace LocalJSX {
'calcite-alerts': CalciteAlerts;
'calcite-loader': CalciteLoader;
'calcite-modal': CalciteModal;
'calcite-switch': CalciteSwitch;
'calcite-tab': CalciteTab;
'calcite-tab-nav': CalciteTabNav;
'calcite-tab-title': CalciteTabTitle;
Expand Down Expand Up @@ -187,6 +205,12 @@ declare global {
new (): HTMLCalciteModalElement;
};

interface HTMLCalciteSwitchElement extends Components.CalciteSwitch, HTMLStencilElement {}
var HTMLCalciteSwitchElement: {
prototype: HTMLCalciteSwitchElement;
new (): HTMLCalciteSwitchElement;
};

interface HTMLCalciteTabElement extends Components.CalciteTab, HTMLStencilElement {}
var HTMLCalciteTabElement: {
prototype: HTMLCalciteTabElement;
Expand Down Expand Up @@ -216,6 +240,7 @@ declare global {
'calcite-alerts': HTMLCalciteAlertsElement;
'calcite-loader': HTMLCalciteLoaderElement;
'calcite-modal': HTMLCalciteModalElement;
'calcite-switch': HTMLCalciteSwitchElement;
'calcite-tab': HTMLCalciteTabElement;
'calcite-tab-nav': HTMLCalciteTabNavElement;
'calcite-tab-title': HTMLCalciteTabTitleElement;
Expand Down
43 changes: 43 additions & 0 deletions src/components/calcite-switch/calcite-switch.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { newE2EPage } from '@stencil/core/testing';
let page, element, switchElement;
beforeEach(async () => {
page = await newE2EPage();
await page.setContent('<calcite-switch text="Test switch"></calcite-switch>');
await page.waitForChanges();
element = await page.find('calcite-switch');
switchElement = await page.find('calcite-switch >>> label');
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with others that the label should not be baked into this component. In the event that decision changes, this should probably be named switchLabel to avoid confusion.

});

describe('calcite-switch', () => {
it('renders', async () => {
expect(element).toHaveClass('hydrated');
});

it('renders text data', async () => {
expect(switchElement.textContent).toEqual(`Test switch`);
});

it('renders switch at correct position', async() => {
element.setProperty('position', 'right');
await page.waitForChanges();
let pageEl = await page.find('calcite-switch >>> .toggle-switch__track');
expect( pageEl).toHaveClass('toggle-switch__track--right');

element.setProperty('position', 'left');
await page.waitForChanges();
expect( pageEl).toHaveClass('toggle-switch__track--left');
});

it('changes the internal state of checked', async () => {
const input = await page.find('calcite-switch >>> input');
switchElement.click();
await page.waitForChanges();
let value = await input.getProperty('checked');
expect(value).toBe(true);

switchElement.click();
await page.waitForChanges();
value = await input.getProperty('checked');
expect(value).toBe(false);
});
});
176 changes: 176 additions & 0 deletions src/components/calcite-switch/calcite-switch.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
// ┌────────────┐
// │ Switches │
//└────────────┘
// ↳ http: //esri.github.io/calcite-web/documentation/components/#switches
// ↳ components → _switches.md
// variables
$switch-width: 36px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest avoiding using sass variable for any values that are only used once in the CSS. The result will be less code, and you avoid having to do a lookup when you're trying to find what a CSS property is set to.
The first five sass variables here are only ever referenced once. Please do a pass on these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt any absolute values or color values to be fed in by consumers if they want different sizes.

$switch-height: 20px;
$switch-bg: #{$lightest-gray};
$switch-border-color: #{$light-gray};
$switch-hover-bg: #{$lighter-gray};
$switch-hover-border-color: #{$light-gray};
$switch-checked-bg: $blue;
$switch-checked-border-color: $dark-blue;
$switch-focus-shadow: 0 0 4px 2px rgba($light-gray,0.9);
$switch-focus-checked-shadow: 0 0 4px 2px rgba($lighter-gray,0.9);
$switch-focus-destructive-checked-shadow: 0 0 4px 2px rgba($lighter-gray,0.9);
$handle-size: 18px;
$handle-top-distance: -1px;
$handle-edge-distance: -1px;
$handle-active-edge-distance: 1px;
$handle-bg: $white;
$handle-border-color: $darkest-gray;
$handle-shadow: 0 1px 1px 0px rgba($darkest-gray,0.2);
$handle-hover-border-color: #{$blue};
$handle-hover-shadow: 0 1px 2px 0px rgba($darkest-gray,0.2);
$handle-checked-border-color: $dark-blue;
$handle-checked-shadow: 0 2px 1px 0px rgba($darkest-gray,0.2);
$handle-active-shadow: 0 3px 1px 0px rgba($darkest-gray,0.2);
$switch-destructive-checked-bg: $dark-red ;
$switch-destructive-checked-border-color: $darker-red;
$handle-destructive-hover-border-color: $dark-red;
$handle-destructive-checked-border-color: $darker-red;
$switch-handle-margin: 18px;

@mixin position-left($left) {
left: $left;
right: auto;

html[dir="rtl"] & {
left: auto;
right: $left;
}
}

@mixin position-right($right) {
right: $right;
left: auto;

html[dir="rtl"] & {
right: auto;
left: $right;
}
}

// our containing block element
.toggle-switch {
display: block;
position: relative;
cursor: pointer;
-webkit-user-select: none;
patrickarlt marked this conversation as resolved.
Show resolved Hide resolved
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
-webkit-tap-highlight-color: transparent;
}

// track
.toggle-switch__track {
position: relative;
display: inline-block;
vertical-align: top;
width: $switch-width;
height: $switch-height;
top: .05em;
background-color: $switch-bg;
border-radius: 30px;
border: 1px solid $switch-border-color;

transition: all 0.25s ease;
// handle
&:after {
position: absolute;
display: block;
content: '';
width: $handle-size;
height: $handle-size;
top: $handle-top-distance;
@include position-left($handle-edge-distance);
background-color: $handle-bg;
border-radius: 30px;
border: 2px solid $handle-border-color;
box-shadow: $handle-shadow;
transition: all 0.25s ease;
}

// alignment fixes for edge
@supports (-ms-ime-align: auto) { & { top: .1em; }}
patrickarlt marked this conversation as resolved.
Show resolved Hide resolved
// alignment fixes for ff
@supports (-moz-appearance: none) { & { top: .1em; }}
// alignment fixes for ios
@supports (-webkit-overflow-scrolling: touch) { & { top: .15em; }}
}

.toggle-switch__input {
@include visually-hidden;

// hover
&:hover + .toggle-switch__track {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems odd to hover over something visually hidden - perhaps the hover belongs on a visual element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

visually hidden is not display none instead it is 1 sq px in size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. But isn't it pretty impossible to get the mouse to hover over on that 1px?

border-color: $switch-hover-border-color;
background-color: $switch-hover-bg;
&:after {
border-color: $handle-hover-border-color;
box-shadow: $handle-hover-shadow;
}
}
// active
&:active + .toggle-switch__track {
box-shadow: $switch-focus-shadow;
&:after {
@include position-left($handle-active-edge-distance);
border-color: $handle-checked-border-color;
box-shadow: $handle-active-shadow;
}
}
// checked
&:checked + .toggle-switch__track {
border-color: $switch-checked-border-color;
background-color: $switch-checked-bg;
&:after {
@include position-right($handle-edge-distance);
border-color: $handle-checked-border-color;
box-shadow: $handle-checked-shadow;
}
}
// checked and active
&:checked:active + .toggle-switch__track {
box-shadow: $switch-focus-checked-shadow;
&:after {
@include position-right($handle-active-edge-distance);
border-color: $handle-checked-border-color;
box-shadow: $handle-active-shadow;
}
}
// unchecked focus
&:focus + .toggle-switch__track {
box-shadow: $switch-focus-shadow;
&:after { border-color: $handle-hover-border-color; }
}
// checked focus
&:checked:focus + .toggle-switch__track {
box-shadow: $switch-focus-checked-shadow;
&:after { border-color: $handle-checked-border-color; }
}
}

.toggle-switch--destructive {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest you overload classes with modifiers (e.g. .toggle-switch.toggle-switch--destructive {}). A modifier is an override, so it makes sense to add specificity to the selector for when a modifier is present. This also prevents this modifier from being misused by limiting it's usage to override only the intended selector.

Also - seems like this selector should be renamed if the property is renamed. I'm also in favor of naming that describes behavior (which should be universally true) over intent (may/may not be true).

& .toggle-switch__input:hover + .toggle-switch__track:after { border-color: $handle-destructive-hover-border-color; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps I'm missing something, but these ampersands are unnecessary aren't they?

& .toggle-switch__input:active + .toggle-switch__track:after { border-color: $handle-destructive-checked-border-color; }
& .toggle-switch__input:checked:active + .toggle-switch__track {
box-shadow: $switch-focus-destructive-checked-shadow;
&:after { border-color: $handle-destructive-checked-border-color; }
}
& .toggle-switch__input:checked + .toggle-switch__track {
background-color: $switch-destructive-checked-bg;
border-color: $switch-destructive-checked-border-color;
&:after { border-color: $handle-destructive-checked-border-color; }
}
& .toggle-switch__input:focus + .toggle-switch__track:after {
border-color: $handle-destructive-hover-border-color;
}
& .toggle-switch__input:checked:focus + .toggle-switch__track {
box-shadow: $switch-focus-destructive-checked-shadow;
&:after { border-color: $handle-destructive-checked-border-color; }
}
}
49 changes: 49 additions & 0 deletions src/components/calcite-switch/calcite-switch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { Component, h, Prop, State, Event, Element, Host } from "@stencil/core";
import { EventEmitter } from "@stencil/state-tunnel/dist/types/stencil.core";
// import { ENTER } from "../../utils/keys";

@Component({
tag: "calcite-switch",
styleUrl: "calcite-switch.scss",
shadow: true
})
export class CalciteSwitch {
@Element() el: HTMLElement;

@Prop() checked = false;

@Prop() color: "red" | "green" = "green";

@State() switched = this.checked;

@Event() switchChange: EventEmitter;
Copy link
Contributor

Choose a reason for hiding this comment

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

Events names are part of the global DOM scope so this could potentially interfere with another 3rd party component which emits a switchChange event. This should probably be something like calciteChange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt calciteChange is confusing name as calcite can be many thing. But I was thinking calciteSwitchChange but felt that this is long enough.

Copy link
Member

Choose a reason for hiding this comment

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

This is something I've been wondering about. Wouldn't we want to model this closely to the stock HTML element that it's using? Like just change. Almost every HTML element can emit a "click" event. I feel like the shared event name is a feature. Wouldn't the shared event name make integration with React easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with Paul too, we are already have calcite in the name of the component so i would like that not to repeat in all event names

Copy link
Contributor

Choose a reason for hiding this comment

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

@paulcpederson @kumarGayu there are 2 problems with just having this event name be change.

  1. change events are already defined in the HTML Living Standard as only being fired from form elements (also defined by the standard). Since <calcite-switch> isn't a form element (as defined in the standard) it shouldn't use change.
  2. It is unclear how <calcite-switch> would play out in something like React which has its own synthetic event system that probably wraps standard browser events. But our change is different then a standard change event. There also isn't a guarantee that this wouldn't break in the future.
  3. If we decide on a non-standard name (changed vs change) there isn't a guarantee that our event name wouldn't conflict with another event name possibly from another library in the future.

As an example, Ionic prefixes all their event names with ion to separate then from regular browser events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that is kind of only option left, I am okay in prefixing calciteChange

@Prop() disabled: boolean = false;
@Prop() name: string;
@Prop() value: string | number | string[];
childInput: any;
childOnchange: any;
@Prop() focus: any;

componentWillLoad(){
this.childInput = this.el.children[0] || (<input type="checkbox" checked={this.switched} disabled={this.disabled} name={this.name} value={this.value}/>);
this.childInput.class = this.childInput.class + "toggle-switch__input";
this.el.innerHTML = ""
this.childOnchange = this.childInput.onChange;
this.childInput.onChange = this.setInputSlot.bind(this);
}

render() {
return (
<Host class={`toggle-switch toggle-switch--${this.color}`} onClick={this.setInputSlot.bind(this)} checked={this.checked} disabled={this.disabled} focus={this.focus} >
{
this.childInput && <slot></slot>
}
<span class={`toggle-switch__track`} />
</Host>
);
}
setInputSlot(event): void {
this.childOnchange && this.childOnchange(event);
this.switchChange && this.switchChange.emit(this.switched);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a little clearer to anyone attaching a listener if we passed them an object with a key rather than just a boolean. Maybe something like switchOn would be better nomenclature than checked for a consuming dev. But either would be a nice boon for readability.

Suggested change
this.switchChange && this.switchChange.emit(this.switched);
this.switchChange && this.switchChange.emit({switchOn: this.switched});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lot of us are having difference of opinion in nomenclature. Initially we thought we will have it similar to HTML tag properties. but now we thought events would clash so thinking of calcite prefix, IMO, we should follow same for all different properties or events.

@macandcheese @patrickarlt @driskull

}
}
Loading