-
Notifications
You must be signed in to change notification settings - Fork 78
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
Switch component for charting basic component #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
export class CalciteSwitch { | ||
@Prop() checked = false; | ||
|
||
@Prop() text = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe set to null by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for string empty makes sense than null so thought of initializing it to truthy empty value. But I am okay to change others also thinks that should be null
return ( | ||
<label class="toggle-switch"> | ||
{this.position === "right" && ( | ||
<span class="toggle-switch-label">{this.text}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something for a larger discussion, but it would be nice to follow BEM naming for css classes. So this would be toggle-switch__label
@Event() switchChange: EventEmitter; | ||
|
||
render() { | ||
const checkedClass = this.switched ? "toggle-switch-input--checked" : ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost BEM naming :) toggle-switch__input--checked
|
||
toggle() { | ||
this.switched = !this.switched; | ||
console.log(this.switchChange); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove console.log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I will remove it,
class={`toggle-switch-track toggle-switch-track--${this.position}`} | ||
/> | ||
{this.position === "left" && ( | ||
<span class="toggle-switch-label">{this.text}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would also be nice to have an CSS map object at the top of the file where all classnames are stored .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this repository I have not seen that pattern so did not follow it :(
If we decide to follow BEM, I can follow for this component as well. |
Thanks for submitting! We'd like to preserve the "destructive" style for actions that we want to caution users about. The styles should be there already, perhaps just a "destructive" prop that can be passed to the switch? |
Thinking about how the integrates with browsers and frameworks a little more I did some digging at how Ionic's
While I think Ionic's approach has merit (this is probably part of the Stencil DS offering) but I think we might be able to do something like this: <calcite-switch> <!-- The custom component would hide the checkbox and present a custom UI -->
<input type="checkbox"> <!-- Angular, ect... would interact with this for the values -->
</calcite-switch> |
@driskull I'm not 100% sure that we need to go full BEM on this. Since styles are scoped to components and then scoped by Shadow DOM (or its polyfills) that we shouldn't any any problems with style conflicts which seems to be the main purpose of BEM. We also don't really have that much CSS per component only ~130 lines of CSS for this component sans variables and utilities. |
@patrickarlt is there any way i can validate my slot to allow only |
I was thinking BEM just for consistency but we can just do that for our other components. |
@driskull changed the classes to BEM style. @patrickarlt I have tried to get it working for both ways if there is slot you can listen on slots. if not you can listen to stencil event. |
@macandcheese I have included the destructive property and the styling is updated accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kumarGayu I can't help but feel that this component is trying to do too much, particularly with the text
and position
properties. I also feel like there are some missed opportunities to improve how this component will integrate with the DOM in general and frameworks.
There are also a few other things:
<calcite-switch>
needs a:focus
state for accessibility<calcite-switch>
should support adisabled
state- We need a discussion around ARIA for the component, possibly just following https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/checkbox_role and applying
display:none
to the passed<input type="checkbox">
to hide it form the accessibility tree.
|
||
@Prop() checked = false; | ||
|
||
@Prop() text = ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text
option feels a little superfulous here. Couldn't we simply say that the <calcite-switch>
compoennts is JUST the switch and let users wrap their own text around it like so:
<label>
Text Before
<calcite-switch>
<input type="checkbox" ...>
</calcite-switch>
Text After
</label>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this is similar to how we'd like to do the checkbox component as well
|
||
@Prop() text = ""; | ||
|
||
@Prop() position: "left" | "right" | "after" | "before" = "left"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to text
couldn't we just let users decide where they want to place this with a standard CSS layout?
<label>
<p style="float: left;"></p>
<calcite-switch style="float: right;">
<input type="checkbox" ...>
</calcite-switch>
</label>
Removing bothposition
and text
would greatly simplify the component implementation and actually give users more flexibility in how they layout <calcite-switch>
relative to the label and surrounding text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickarlt I have decided text to be inside the component because
- Switch component can not exist with out explaining what is the purpose of it.
- Switch component wrapping in label element might not work as expected.
still if you think switch would stand alone do the purpose then making it all alone will end up making us having one more component which is widely used.
When we combine text and switch then position is required position the switch. else we can remove position property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree that from an "end-user UI" perspective, labels or descriptions should accompany switches, I think we should leave the positioning and text to the implementer to make the component as light as possible. Perhaps that's a place where we'll want to have some extra documentation about best practices.
|
||
@Prop() position: "left" | "right" | "after" | "before" = "left"; | ||
|
||
@Prop() destructive = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
destructive
seems like an unclear property name. I think I would rather follow what @macandcheese did for <calcite-alert>
and simply use color names and not prescribe meaning to them. I think @Prop() color:"red" |"blue" = "blue"
would be a better way forward here and be more consistent with established components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ported that from calcite web 2.0. Also, Color would definitely confuse why red and why blue, may be intuitive property would help consumers. do you suggest any other property name than destructive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we had used "intent" instead of color in that particular case because we wanted to avoid folks just using red switches when the action wasn't inherently destructive. Probably a larger discussion since many components will have some type of accent / status color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that trying to implement color themes though properties names like destructive
is really confusing. When I saw this I thought this property might destroy the switch after I toggled it. For clarity to the developer consuming this I think this is best used as description of what it does rather then what we intent it to be used for. "This property turns the switch red" (clear) vs "this makes the switch destructive" (unclear).
The also works for other things like alerts. https://esri.github.io/calcite-web/documentation/components/#alerts instead of success, warning, info and error alerts we simply use red, blue, green and yellow and leave it up to the developer to give the basic colors meaning.
As a example should we (the Calcite Components team) decide what color the switch should be if we replaced the checkbox here:
Neither enabling or disabling delete prevention is "destructive" but disabling it MIGHT be. Should we really be deciding this for teams?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm, I feel we are pushing the overhead of deciding to users(consumers). But for colors it looks fine, as this colors would mean different to different consumers. Therefore, using intent to describe our property may not be that great at this base level. Should this be followed at every components in this repo? we should use what it does
for property names rather than what intent it is for
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed... let's just use "color" here :)
|
||
@State() switched = this.checked; | ||
|
||
@Event() switchChange: EventEmitter; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
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 usechange
.- 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 ourchange
is different then a standardchange
event. There also isn't a guarantee that this wouldn't break in the future. - If we decide on a non-standard name (
changed
vschange
) 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.
There was a problem hiding this comment.
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
{(this.position === "right" || this.position === "after") && ( | ||
<span class="toggle-switch__label">{this.text}</span> | ||
)} | ||
<input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this input should be present here. Looking at the Ionic impliment for <ion-switch>
they render a hidden input on every call to render()
https://github.com/ionic-team/ionic/blob/master/core/src/components/toggle/toggle.tsx#L210 and https://github.com/ionic-team/ionic/blob/3d656ac3127278a7c9d3d4808c2806fde71e5d2b/core/src/utils/helpers.ts#L25. They then sync that input with the state of the <ion-toggle>
. This hidden input lives outside the ShadowDOM so it take part in interaction with <form>
elements.
Given that we also want to allot users to pass a <input type="checkbox">
into the <slot>
I think we should do the following:
- Add lifecycle hooks for
componentWillLoad()
andcomponentWillUpdate()
inside the hook we should:- Check if there is a checkbox input as a child of
this.el
. - Treat that checkbox as the "source of truth" and sync the
name
,disabled
andvalue
andchecked
properties from it. - This should keep the state of
<calcite-switch>
in sync with the users<input type="checkbox">
- Check if there is a checkbox input as a child of
- On every
render()
do the following:- Check if there is an input as a child of
this.el
if there isn't one create a hidden input (like Ionic) and treat<calcite-switch>
as the source of truth and sync thename
,disabled
andvalue
andchecked
values down to it. This hidden input should be a child of the<calcite-switch>
but not part of the ShadowDOM.
- Check if there is an input as a child of
- Inside the
componentWillLoad()
hook we should also setup and event listener on the<input type="checkbox">
if one was provided and updated thechecked
property of<calcite-switch>
based on it. - To do this name
,
disabledand
valuewill all have to be added as properties of
`.
Doing this will allow users to do the following:
- Pass an
<input type="checkbox">
that acts as the "source of truth" for the<calcite-switch>
. This allows frameworks (like Angular, ect...) to interact using their standard tools with the checkbox like<input type="checkbox" [(ngModel)]="booleanPropInMyAngularComponent">
and have that drive the<calcite-switch>
- If a user doesn't pass a
<input type="checkbox">
then<calcite-switch>
acts as the soruce of truth but maintains a hidden form element so that any<form>
element will still appear to have a value for<calcite-switch>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Basically, we should support anything set on slot should reflect on switch. but, now it is other way. Would not this be confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we also want to allot users to pass a
<input type="checkbox">
into the<slot>
Is that a given?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of these things (the switch or supplied checkbox) should "win"over the other if the settings are out of sync. For example what should that state of the checkbox be here:
<calcite-switch checked disabled name="bar" value="baz">
<input name="foo" value="woo"> <!-- keep in mind the framework like Angular will bind its values here -->
</calcite-switch>
class="toggle-switch__input" | ||
checked={this.switched} | ||
onChange={this.setInputSlot.bind(this)} | ||
onKeyPress={(event: KeyboardEvent) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since my above comment is all about removing this (or potentially replacing it with a hidden input outside the ShadowDOM) <calcite-switch>
will need to have a tabindex
on the <Host>
element and listen for the keydown event like how <calcite-tab-title>
does https://github.com/ArcGIS/calcite-components/blob/master/src/components/calcite-tab-title/calcite-tab-title.tsx#L67-L80.
Looks pretty good, but this PR is definitely bringing up some questions we'll need to solve on the project as a whole:
Ideally (and this is pie-in-the-sky land) our component would be pretty much a drop in replacement for what we're replacing, IMO. So as a developer, I previously had:
I should just be able to drop in:
And leave all of my event listeners/handling code as-is and it would just work. Is this even possible? Also, I've moved the switch issue to "in progress" on the project board. |
It looks like the :focus states aren't being applied when the switch 'is-active' .... we at some point hope to have better focus styles across ALL components, but for now we should be able to use the existing focus and active styles in the CW component: https://esri.github.io/calcite-web/documentation/components/#switches Note that it is grey for unchecked switches of any type, red for checked "destructive / red" and blue for checked default, to match the style of the checked item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall - some styling and class name things - thanks for being the guinea pig on this, most of these are framework-level decisions that have yet to be decided upon, and this gives us a venue for those conversations :)
return [ | ||
<label | ||
class={ | ||
this.destructive ? "toggle-switch--destructive" : "toggle-switch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be an additional modifier class: "toggle-switch toggle-switch--destructive".
Currently, "destructive" switches don't receive the "toggle-switch" class and therefore do not have cursor: pointer
, or any other base toggle-switch styles.
It may also be a little cleaner to style via a prop, instead of a class, ie:
:host([color="red"]) {
...
}
Since event names are part of the global name space I think we should prefix all event names with
@paulcpederson I don't know. I don't think that we should also assume that it would be totally cross frameworks either due to different implimentations of event system ect... What I have been advocating for is for either of these to be eqiviliant: <calcite-checkbox checked disabled></calcite-checkbox> and <calcite-checkbox checked disabled>
<input type="checkbox" checked disabled>
</calcite-checkbox>
<calcite-checkbox checked disabled>
<input type="checkbox" checked disabled [(ngModel)]="somePropInMyAngularComponent">
</calcite-checkbox> Ionic gets around this by publishing seperate wrappers for So there are 3 paths forward:
|
there are 2 problems with this.
for ex: <calcite-switch>
<input class="cssClass"/>
</calcite-switch>
.cssClass{
this styles may not work 100%.
}
if I miss something please do correct me.
keeping both in sync if child is present is kind of duplicate effort in terms of memory and code. This patter adds dev efforts in many of the component we develop. |
Summarizing decisions from this thread (good discussion!):
Does all that sound reasonable to everybody? |
@paulcpederson thanks for summarizing - that all LGTM. I'll update Alerts and the other components I have in progress to reflect these decisions. We should also probably decide on a code format style / linter and then put together a quick readme for all these high level considerations. I have no opinion on that, as long as it's consistent. Maybe use Hound et al. for pull requests. |
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'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. I have a couple recommendations though.
// ↳ http: //esri.github.io/calcite-web/documentation/components/#switches | ||
// ↳ components → _switches.md | ||
// variables | ||
$switch-width: 36px; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
// spacing for label | ||
.toggle-switch__label { | ||
width: calc((100% - 3em) - .5em); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just subtract 3.5em?
@include visually-hidden; | ||
|
||
// hover | ||
&:hover + .toggle-switch__track { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
} | ||
} | ||
|
||
.toggle-switch--destructive { |
There was a problem hiding this comment.
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--destructive { | ||
& .toggle-switch__input:hover + .toggle-switch__track:after { border-color: $handle-destructive-hover-border-color; } |
There was a problem hiding this comment.
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?
input.checked = this.switched; | ||
input.dispatchEvent(evt); | ||
} | ||
this.switchChange && this.switchChange.emit(this.switched); |
There was a problem hiding this comment.
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.
this.switchChange && this.switchChange.emit(this.switched); | |
this.switchChange && this.switchChange.emit({switchOn: this.switched}); |
There was a problem hiding this comment.
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.
@@ -15,7 +15,11 @@ | |||
padding: 1rem; | |||
} | |||
</style> | |||
|
|||
<script> | |||
function sayHi(val) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val not used on line 292. Might even be able to just hardcode console.log('hi') as the funciton passed to onChange to clean this up a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure 👍
I think this applies to more than just colors. I think the general idea here is that our names of properties, methods, events, etc. should describe behavior rather than semantic intent. This makes it very simple for anyone utilizing the component to know what something is for (prop), what it does (method), or what it's doing (event). I support this idea. |
@pr3tori4n good review. A lot of the Sass comments are from the CW code, but this will be a good opportunity to clean up some of that Sass code as we're porting it over. |
After having all the discussions, this component now splits into 2 components, <calcite-label>
<slot name="text"><span></span></slot> // based on position
<calcite-switch calcite-switch-change=()=>{} calcite-switch-focus=()=>{}>
<input type="checkbox" > //optionally. and sync these properties back and forth from calcite-switch.
</calcite-switch>
<slot name="text"><span></span></slot> // based on position
</calcite-label> I will start working on this 👍 but one question arose from @pr3tori4n discussion that should the properties be named based off the component name? |
No I don't think we want a |
Hmm, I kind of feel like label should just be left as plain HTML and not a web component.
For checked, that's also semantically standard, not sure if we'd want to change that... |
@paulcpederson / @macandcheese today it might not have any style or purpose. having that as a separate component can add values in future. As all component that we use is under one central code. |
In a situation where Calcite Components is a more 1:1 re-creation of CW that includes non-functional components like buttons, and inputs (Ionic as an example does), I could see labels being appropriate to add, but with our current roadmap it seems a little out of scope since it's just recreating HTML with no interactivity. Neither Material, Carbon seem to use custom label components. Perhaps in the future if CC moves towards Ionic in its structure, a custom label component might be more fitting and appropriate to add in at that time? |
@macandcheese I agree. but in that rewriting may be little more time than adding just now one component. if adding that as a component is not too much work then we can think of moving it now. I believe that when we have to rewrite then we will have that as a reason to not to have that new component. :( |
We can close this for now, as patrick has created conventions as readme and also we have a sample form component. |
feature(tokens): Design Token Updates
Added Calcite-Switch component as per #3
and as per design
https://esri.github.io/calcite-web/documentation/components/#switches