This repository has been archived by the owner on Dec 8, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Toggle Switch - Phase 2 docs #109
Toggle Switch - Phase 2 docs #109
Changes from 7 commits
c5d1d2f
0d91d9d
8fa6c76
8d54e8b
bccee86
cce4f9e
df16779
2ef5976
ff72157
a19036d
a76be76
c18222f
3299e53
22c0f5a
c924544
003de9a
043c25d
cdef3fe
3f856d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered 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.
Looking at this again, the opening clause is nonsensical. If the scenario does NOT allow for toggles to become disabled, then toggles shouldn't be used. ... I think what we mean is something more like
To make it clear what happens when users select toggles, you can include different text labels for the selected and unselected states.
Or something like that.Again, if we change this, we should probably bounce it off of Adam or Todd.
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 went ahead and just made the change, but I can easily just put it back to what it was if they disagree.
@Blackbaud-AdamFunderburk or @Blackbaud-ToddRoberts Any strong thoughts on this change?
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.
@Blackbaud-AdamFunderburk and @Blackbaud-ToddRoberts, just checking back in to see if you guys are OK with this change.
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 the new wording is generally good. @Blackbaud-AdamFunderburk I don't remember why we decided that disabled toggles always need labels, even in grids, but I do remember that it was intentional. Should we add that back in?
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's important to maintain the point that labels should be used if the switches can be disabled.
"If toggle switches can be disabled, include individual text labels to reflect their state."
I understand how tangled that makes the guidelines around the text label, and I'm not sure quite how to optimize them.
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 Development tab content is missing. We need to add JSDoc comments. We just need to copy the content from the existing Development tab (minus the
This property accepts...
part) and paste it as JSDoc comments https://github.com/blackbaud/skyux-forms/blob/master/src/app/public/modules/toggle-switch/toggle-switch.component.ts. If you need an example, see the JSDoc comments for dropover at https://github.com/blackbaud/skyux-popovers/blob/master/src/app/public/modules/dropdown/dropdown.component.ts.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.
Also, it looks like we only have one code example. In the old demo, we had an example of a reactive form and a template-driven form. Should we have both here as well? Might be worth asking Steve about that.
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 copied the code example from the specs that @Blackbaud-AdamFunderburk created, I know there's a few of them that are a bit different from the old code demos. I assumed the new ones had already been vetted, perhaps adam can weigh in?
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 template-driven form vs. reactive form difference might not matter in phase 2 docs. We can ask Adam at the docs check-in on Tuesday.
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.
✔️