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

Fixed - IconSwitch - different background color to indicate checked/unchecked #131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

goguul
Copy link
Member

@goguul goguul commented Jul 8, 2020

Showing different background color using fill prop is existing functionality. Fixed where it has been removed.

Useful when same Icon is used for both checked/unchecked.

Currently fill prop is used in Saved Views (IncludeScopeToggleButton). This change is needed for S-59565 (Milestone Toggle in Roadmapping).

…nchecked.

Showing different background color using fill prop is existing functionality.
Fixed where it has been removed.

Useful when same Icon is used for both checked/unchecked.

Currently fill prop is used in Saved Views (IncludeScopeToggleButton).
This change is needed for S-59565 (Milestone Toggle in Roadmapping).
Copy link

@kmmarsh kmmarsh left a comment

Choose a reason for hiding this comment

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

This might be a good opportunity to remove the sliding hover on this switch that has been removed in the rest of Core

@goguul
Copy link
Member Author

goguul commented Jul 13, 2020

This might be a good opportunity to remove the sliding hover on this switch that has been removed in the rest of Core

Removed the sliding hover on IconSwitch and IconButton

Copy link
Contributor

@walkerrandolphsmith walkerrandolphsmith left a comment

Choose a reason for hiding this comment

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

Please include the relevant information contained here https://paper.dropbox.com/doc/Toggle-Behavior--A3pQUObTqVMJewSmAvm_X5xFAg-gPTJvQKRj2NfporZ6wI2k in the README. If that information is no longer accurate then a designer can indicate that it is stale in their review. Thanks!

@walkerrandolphsmith
Copy link
Contributor

It looks like this pr does not violate those design decisions made by @faudau but the mockups do. I think we should keep that in mind when moving forward with S-59565.

@mhh
Copy link
Contributor

mhh commented Jul 13, 2020

@walkerrandolphsmith can you clarify what you mean? the Icon toggle section says: "We represent the toggle with icons that can be empty (off) / filled (on)". To me, it looks like this change and the mockups for S-59565 adhere to that.

@goguul
Copy link
Member Author

goguul commented Jul 14, 2020

@walkerrandolphsmith Thanks for your feedback. We had discussion with @jerryo regarding this change. He added a comment in https://paper.dropbox.com/doc/Toggle-Behavior--A3pQUObTqVMJewSmAvm_X5xFAg-gPTJvQKRj2NfporZ6wI2k

From Jerry's point of view, the design doc didn't clearly mention what “filled” and “empty” is. Whether it means to fill the icon container background like in this PR or fill the Icon (like saved views favorite icon filled-star.svg)
image

We decided to go with existing approach to have two different Icons milestone.svg & filled-milestone.svg (not available currently need to create).

Please provide your suggestion.

If Icon with different background color is not needed, I can modify the code change in this PR to remove the fill prop in IconSwitch

@walkerrandolphsmith
Copy link
Contributor

The decision that was attempted to be captured in the docs was that the icon itself would be filled and not the button. This is demonstrated in the SavedViews feature which prompted the documentation to be written.

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

4 participants