Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Contrib > Toggle switch #54

Merged
merged 18 commits into from
Jul 23, 2019
Merged

Contrib > Toggle switch #54

merged 18 commits into from
Jul 23, 2019

Conversation

Blackbaud-SteveBrush
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush commented Jul 18, 2019

@codecov
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

Merging #54 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #54   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     16    +3     
  Lines         539    622   +83     
  Branches       86     91    +5     
=====================================
+ Hits          539    622   +83
Impacted Files Coverage Δ
...les/toggle-switch/toggle-switch-label.component.ts 100% <100%> (ø)
...c/modules/toggle-switch/toggle-switch.component.ts 100% <100%> (ø)
...blic/modules/toggle-switch/toggle-switch.module.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68f6672...1ddcd17. Read the comment docs.

Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

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

Caught one small style thing

this.ngUnsubscribe.complete();
}

public writeValue(value: boolean) {

Choose a reason for hiding this comment

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

Missing a return type here

Choose a reason for hiding this comment

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

Also a few others missing in this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@Blackbaud-TrevorBurch
Copy link
Member

@Blackbaud-SteveBrush @Blackbaud-JackMcElhinney I apologize if this has been discussed or should be clear but I noticed that when you click on the toggle that the entire rectangle "button" around the toggle highlights in the focused state instead of it being indicated on the toggle itself. Just wanted to verify that this is correct.

background-color: $sky-background-color-neutral-light;
padding: 1px;
border-radius: $indicator-size + 2;
width: $indicator-size * 2 + 2;
Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman Jul 22, 2019

Choose a reason for hiding this comment

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

The widths seem to be effected by a shrinking parent (or window). For example, if you load the visual test and resize the window small enough, it will squeeze the toggle switch's background. Disabled switches seem to be impacted more. Is there any way to make it more resilient to width changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

border-radius: 100%;
box-shadow: 0px 1px 2px 0px rgba(0, 0, 0, 0.75);
background-color: $sky-color-white;
transition: left 150ms;
Copy link
Contributor

Choose a reason for hiding this comment

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

This transition is also happening when the control first loads (Firefox). Should we prevent that from happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! Done.

Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

This looks great, @Blackbaud-JackMcElhinney . Great work! Just a couple questions for you above. Also, why are we deleting all the old baselines?

@Blackbaud-AlexKingman
Copy link
Contributor

Blackbaud-AlexKingman commented Jul 22, 2019

Maybe this is a question for @Blackbaud-ToddRoberts, but I noticed there's not way to visually tell the difference between disabled & selected, vs disabled & unselected, unless you inherently know that a switch pushed to the right is "selected". Does that cause any concern?

Screen Shot 2019-07-22 at 10 29 48 AM

@Blackbaud-ToddRoberts
Copy link

@Blackbaud-AdamFunderburk I know we talked about disabled state styling at some point but can't remember the outcome. Would the guideline be that if your toggle can have a disabled state you should always include a label?

@Blackbaud-AdamFunderburk
Copy link
Contributor

Todd and I made some revisions to the guidelines around the text label:

image

@Blackbaud-SteveBrush
Copy link
Member Author

@Blackbaud-TrevorBurch I confirmed with @Blackbaud-ToddRoberts and the focus styling is appropriate, at least for the time being.

@Blackbaud-SteveBrush
Copy link
Member Author

@Blackbaud-AlexKingman I had to remove the screenshots because the background color was changed from gray to white.

@Blackbaud-SteveBrush Blackbaud-SteveBrush merged commit 2ced7e9 into master Jul 23, 2019
@Blackbaud-SteveBrush Blackbaud-SteveBrush deleted the contrib-toggle-switch branch July 23, 2019 14:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants