Toggle Switch - Phase 2 docs #109
Conversation
Blackbaud-DeniseCisneros
requested review from
blackbaud-johnly,
Blackbaud-SteveBrush and
Blackbaud-AdamFunderburk
Blackbaud-DeniseCisneros
requested a review
from Blackbaud-AlexKingman
as a code owner
|
/azp run |
There was a problem hiding this comment.
I made a handful of docs-related comments. We also need to figure out why checks are failing. Looking through the error logs, it looks like it's related to e2e tests and a bunch of screenshots that don't match as expected. I'm not really sure what we need to do to fix that problem, so we probably need to talk to a dev on the team.
| When toggles are in grid columns, the text label is not required if you use a column heading. Use text labels for all other states. | ||
| </p> | ||
| <p> | ||
| If the scenario allows for toggles to become disabled, always include a text label on each toggle. |
There was a problem hiding this comment.
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.
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.
@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.
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.
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.
| <sky-docs-code-example heading="Toggle" | ||
| sourceCodePath="src/app/public/plugin-resources/code-examples/toggle-switch/basic"></sky-docs-code-example> | ||
| </sky-docs-code-examples> | ||
|
|
There was a problem hiding this comment.
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.
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.
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.
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.
✔️
|
I'll work today to see if I can figure out why the checks are failing. It's strange, that check is passing on my local machine. |
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 913 913
Branches 128 128
=========================================
Hits 913 913
Continue to review full report at Codecov.
|
* Bump acorn from 6.4.0 to 6.4.1 (#110) Bumps [acorn](https://github.com/acornjs/acorn) from 6.4.0 to 6.4.1. - [Release notes](https://github.com/acornjs/acorn/releases) - [Commits](acornjs/acorn@6.4.0...6.4.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * added license (#112) Added license to address blackbaud/skyux2-docs#894 * Toggle Switch - Phase 2 docs (#109) * First pass of adding phase 2 of toggle switch docs * Added docs tools styles & related components * Add design demo * Clean up code for e2e check * Remove unused imports * Add code demo * Update the pact package * Fix visual * Implemented feedback * Fixed formatting issues * updated dev dependencies * Fix demo * Add default value for `checked` * Added description for toggle switch label * Added description for SkyToggleSwitchChange * Updating with revised text Co-authored-by: Somalia Jamall <somalia.jamall@blackbaud.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> Co-authored-by: Blackbaud-SteveBrush <steve.brush@blackbaud.com> * A11y improvements (#114) * a11y improvements * Remove codeowners * Upgrade packages * Fix screenshots * Fix visual * Revert image deletion Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Denise Pena <Denise.Pena@blackbaud.me> Co-authored-by: Somalia Jamall <somalia.jamall@blackbaud.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com>
* [rc] Angular 9; drop RxJS 5 (#102) * Updated changelog/package.json for 4.0.0-rc.0 release * [rc] Merge master; test fixtures (#115) * Bump acorn from 6.4.0 to 6.4.1 (#110) Bumps [acorn](https://github.com/acornjs/acorn) from 6.4.0 to 6.4.1. - [Release notes](https://github.com/acornjs/acorn/releases) - [Commits](acornjs/acorn@6.4.0...6.4.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * added license (#112) Added license to address blackbaud/skyux2-docs#894 * Toggle Switch - Phase 2 docs (#109) * First pass of adding phase 2 of toggle switch docs * Added docs tools styles & related components * Add design demo * Clean up code for e2e check * Remove unused imports * Add code demo * Update the pact package * Fix visual * Implemented feedback * Fixed formatting issues * updated dev dependencies * Fix demo * Add default value for `checked` * Added description for toggle switch label * Added description for SkyToggleSwitchChange * Updating with revised text Co-authored-by: Somalia Jamall <somalia.jamall@blackbaud.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> Co-authored-by: Blackbaud-SteveBrush <steve.brush@blackbaud.com> * A11y improvements (#114) * a11y improvements * Remove codeowners * Upgrade packages * Fix screenshots * Fix visual * Revert image deletion Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Denise Pena <Denise.Pena@blackbaud.me> Co-authored-by: Somalia Jamall <somalia.jamall@blackbaud.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com> * Updated changelog/package.json for 4.0.0-rc.1 release * [rc] Added type for `errorType` (#118) * Upgrade * Update changelog/package.json for 4.0.0 release * Updated changelog * Fix changelog * Update date and wording tweak Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Denise Pena <Denise.Pena@blackbaud.me> Co-authored-by: Somalia Jamall <somalia.jamall@blackbaud.com> Co-authored-by: John Lyons <John.Lyons@blackbaud.com>
Phase 2 of Toggle Switch docs. Pending demo and code examples