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

Calcite switch #33

Merged
merged 19 commits into from
Jun 5, 2019
Merged

Calcite switch #33

merged 19 commits into from
Jun 5, 2019

Conversation

patrickarlt
Copy link
Contributor

In an attempt to get <calcite-switch> (#24) merged I spent some time this morning to refactor it and open a new PR.

This PR incorporates as much feedback as I could gather from #24 and we can use it as a base for a new review.

<div class={`toggle-switch__track toggle-switch__track--${this.color}`}>
<div class="toggle-switch__handle" />
</div>
<slot />
Copy link
Contributor

Choose a reason for hiding this comment

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

If the user passes in a checkbox to the slot, how does this behave from an a11y perspective? The rendered DOM would be something like:

<calcite-switch role='checkbox' tabindex='0'>
  <div class='toggle-switch__track toggle-switch__track--blue'}>
    <div class="toggle-switch__handle" />
   </div>
  <input type='checkbox' />
<calcite-switch/>

Which semantically has a checkbox input inside of a checkbox input.

We may need additional logic in setupProxyInput like ionic uses in its renderHiddenInput helper to ensure there's always one "real" hidden checkbox.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The input passed into the <slot> is hidden by https://github.com/ArcGIS/calcite-components/pull/33/files#diff-ef2ae6405313ac1fd653283d6be2050aR1 so it won't appear in the accessibility tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may need additional logic in setupProxyInput like ionic uses in its renderHiddenInput helper to ensure there's always one "real" hidden checkbox.

Thinking about this we probably do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would probaly make sure stuff like this worked:

<label>
  Clicking the label should toggle the switch
  <calcite-switch checked></calcite-switch>
</label>

this.switched = !this.switched;
} else if (!this.inputProxy) {
this.switched = !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.

both the blocks are similar, and are we not supporting this focus and active?

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 the switch is contained within a <label> and the switch is clicked there are actually 2 click events fired. 1 on the checkbox and one on the switch. If there is no passed input there is only 1 click event fired. I might be able to resolve this be adding a default checkbox like Ionic https://github.com/ArcGIS/calcite-components/pull/33/files/128c553fdb5ce060eab1b62a429270819e73e4d6#r290499524

Copy link
Contributor

@kumarGayu kumarGayu left a comment

Choose a reason for hiding this comment

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

@patrickarlt thanks for getting us this demo component. 👍 Looks good but we need some extra functionalities as well.

@patrickarlt
Copy link
Contributor Author

@kumarGayu @vcolavin I've added some more code to this. In cases where a user does not pass an input a hidden <input type="checkbox"> is created for them so we can still maintain interactions with <form> and <label> this also means adding a name and value prop to <calcite-switch> that get synced down the input.

I do feel like this needs a disabled state now...

}

// bind the proxy input to update our state
this.switched = this.inputProxy.checked;
Copy link
Contributor

Choose a reason for hiding this comment

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

this.name = this.inputProxy.name this.value = this.inputProxy.value isn't it required? for syncing up to calcite-switch, I feel both of calcite-switch and input have sync'ed values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kumarGayu My is that <calcite-switch> should support all the basic checkbox properties. So value, disabled, switched (syncs with checked) and name. Syncing name and value down from <calcite-switch> isn't strictly required but it makes everything show up nice when you do things like:

<form>
  <calcite-switch name="enableThing" value="true"></calcite-switch>
</form>

Will cause the form to submit with enableThing=true and for a enableThing input to appear in form.elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure make sense 👍

@kumarGayu
Copy link
Contributor

I feel we have to document this way of sending <input /> as child Appropriately with a mention all the properties should send to children <input /> than <calcite-switch />

This works fine

<calcite-switch>
  <input id="proxySwitchCheckbox" type="checkbox" checked/> // as checked is in side
</calcite-switch>

** this may not work correct me if I miss something **

<calcite-switch id="basicSwitch" switched="true">
   <input id="proxySwitchCheckbox" type="checkbox" />// input is not checked even though switched = true
</calcite-switch>

@patrickarlt patrickarlt merged commit 187ef4f into master Jun 5, 2019
@julio8a julio8a mentioned this pull request Aug 20, 2019
10 tasks
@paulcpederson paulcpederson deleted the calcite-switch branch October 29, 2019 21:05
alisonailea added a commit that referenced this pull request Aug 29, 2023
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.

None yet

3 participants