WIP? Add switch component from from Patternfly 4 #767
Conversation
wesruv
force-pushed
the
736-add-pfe-switch-component
branch
from
7332f5e
to
564fb2b
Compare
|
@castastrophe I think I got everything you noted, working on writing tests now. |
| BoxShadow: '0 0.0625rem 0.0625rem 0rem rgba(3, 3, 3, 0.05), 0 0.25rem 0.5rem 0.25rem rgba(3, 3, 3, 0.06)', | ||
| ), | ||
| toggle: ( | ||
| BackgroundColor: #d2d2d2, // @todo - didn't find a great color to match PF4 |
There was a problem hiding this comment.
@castastrophe couldn't find a color that made sense for this, any thoughts?
There was a problem hiding this comment.
I like your todo, we can look into if we're missing a color variable from the map or if we need to update the colors in the map currently at a later date. 👍
| @@ -0,0 +1,4 @@ | |||
| <div class="pfe-switch"> | |||
There was a problem hiding this comment.
I'm not sure this div is necessary. You can style the :host, <pfe-switch>, directly in the stylesheet by using :host without needing to assign a class. If you really want the class, in the JS you could add the class to the host element too.
There was a problem hiding this comment.
Styling tags makes me itchy... but you're right.. old habits, something something
| connectedCallback() { | ||
| super.connectedCallback(); | ||
|
|
||
| const $pfeSwitch = this.shadowRoot.querySelector(".pfe-switch"); |
There was a problem hiding this comment.
The $name syntax is a jQuery convention to indicate a jQuery element variable but would not make sense in this context since we do not use jQuery in the project. The convention we've been using for shadow elements is this._pfeSwitch = this.shadowRoot.querySelector(".pfe-switch");. If you remove the div from the template, you can reference the host simply with this and there's no need to get the shadow element at all.
| "title": "Attributes", | ||
| "type": "object", | ||
| "properties": { | ||
| "form": { |
There was a problem hiding this comment.
The attributes section should list out all the data attributes supported by your component: message-on, message-off, hide-label. The form indication could go in your slot array above instead of referencing a raw field, you could indicate form to tell the component that it only accepts a form element in the slot.
There was a problem hiding this comment.
Is there an example or existing component I can code-monkey off of?
| const $pfeSwitch = this.shadowRoot.querySelector(".pfe-switch"); | ||
|
|
||
| // Get settings from attributes | ||
| const settingAttributes = [ |
There was a problem hiding this comment.
This is done for you by importing the schema. Any attributes defined in your schema will be attached to your component for you by logic built into pfelement. You can access that information using this.props["message-on"] which will list out the object for that attribute. Each attribute has title, type, enum (if defined), default (if defined), a boolean prefixed value (does it have pfe at the beginning of it), and the actual value assigned (if the user has added that attribute to the component).
| // Check for label text | ||
| if (!this.label.textContent.trim().length) { | ||
| console.error( | ||
| "pfe-switch: Label text not found. Form elements require meaningful label text for accessibility. The label can be visually hidden, but must exist." |
There was a problem hiding this comment.
| "pfe-switch: Label text not found. Form elements require meaningful label text for accessibility. The label can be visually hidden, but must exist." | |
| `${this.tag}: Label text not found. Form elements require meaningful label text for accessibility. The label can be visually hidden, but must exist.` |
Using the variable helps in case the component is renamed.
| if (this.settings["hide-label"]) { | ||
| this.label.classList.add("pfe-sr-only"); | ||
| const switchIcon = document.createElement("span"); | ||
| switchIcon.classList.add("pfe-switch__checkmark"); |
There was a problem hiding this comment.
| switchIcon.classList.add("pfe-switch__checkmark"); | |
| switchIcon.classList.add(`${this.tag}__checkmark`); |
Suggest using the variable instead of hardcoding the component name.
| this.checkbox.addEventListener("change", this._updateSwitchState); | ||
| } | ||
|
|
||
| // disconnectedCallback() {} |
There was a problem hiding this comment.
Any event listeners you've attached above should be disconnected here.
|
Link to the preview: https://deploy-preview-767--happy-galileo-ea79c4.netlify.com/elements/pfe-switch/demo/ Man this is a gorgeous component!
|
| } | ||
|
|
||
| .pfe-switch__checkmark { | ||
| // Checkmark is built with CSS instead of using an image |
wesruv
force-pushed
the
736-add-pfe-switch-component
branch
from
8f58f1a
to
ed41624
Compare
wesruv
changed the title
wesruv
changed the title
| cursor: pointer; | ||
|
|
||
| // Toggle container | ||
| &:before { |
There was a problem hiding this comment.
Unfortunately, this type of selector isn't supported in Safari.
::before and ::after selectors inside ::slotted don't work.
WICG/webcomponents#655
There was a problem hiding this comment.
We'll need to come up with an alternative approach. I'm going to investigate moving light DOM content to the shadow DOM.
:before and :after pseudo selectors don't work on slotted content in Safari. With this change, I move the knob and knob container to the shadow root and apply the same css to get the same look as before
assignedElements is not supported in IE11. Switching to assignedNodes gets this working in IE11
Using a setter to react to changes the checked value. Also, I added something I'm not quite sure about. I don't like how the checked attribute on the input doesn't stay in sync with the checked value. I know the docs reflect this on MDN (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox#attr-checked) but it still bugs me.
Whenever you change the value of a CSS variable, IE11 will always use the last value that was set which is, of course, problematic. This hardcodes some values for IE11 so it's at least legible.
:before and :after pseudo selectors don't work on slotted content in Safari. With this change, I move the knob and knob container to the shadow root and apply the same css to get the same look as before
assignedElements is not supported in IE11. Switching to assignedNodes gets this working in IE11
Using a setter to react to changes the checked value. Also, I added something I'm not quite sure about. I don't like how the checked attribute on the input doesn't stay in sync with the checked value. I know the docs reflect this on MDN (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox#attr-checked) but it still bugs me.
Whenever you change the value of a CSS variable, IE11 will always use the last value that was set which is, of course, problematic. This hardcodes some values for IE11 so it's at least legible.
Pfe switch updates
|
I'm going to close this out until we have time to revisit this PR |
#736
Testing instructions
Be sure to include detailed instructions on how your update can be tested by another developer.
Browser requirements
Your component should work in all of the following environments:
Ready-for-merge Checklist
Be sure to share your updates with the [email protected] mailing list!