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

add option for input validation foreground color #57734

Merged
merged 6 commits into from Sep 5, 2018

Conversation

Projects
None yet
2 participants
@ParkourKarthik
Contributor

ParkourKarthik commented Sep 1, 2018

This closes #57536

@bpasero I've made changes only related to "find Input control".
Review and let me know if similar changes could be applied to all other input validations.

@bpasero bpasero self-assigned this Sep 1, 2018

@bpasero bpasero added this to the September 2018 milestone Sep 1, 2018

@bpasero

@ParkourKarthik unless I am mistaken, these colors should not have any default set, otherwise we change the semantics, aren't we? Today I do not think we apply any particular foreground color by default and I think we should only do so if the theme defines this color explicitly.

Also, we need to support this color everywhere where the correlating background color is passed through, so please find all occurrences of where input background colors are used and pass them through.

@ParkourKarthik

This comment has been minimized.

Show comment
Hide comment
@ParkourKarthik

ParkourKarthik Sep 5, 2018

Contributor

@bpasero I've set the default values to null. I believe that is meant to be done when no actual defaults are to be set.

Note: I've not added any Validation Foreground colors to the below as there should not be defaults for Foreground.

const defaultOpts = {
inputBackground: Color.fromHex('#3C3C3C'),
inputForeground: Color.fromHex('#CCCCCC'),
inputValidationInfoBorder: Color.fromHex('#55AAFF'),
inputValidationInfoBackground: Color.fromHex('#063B49'),
inputValidationWarningBorder: Color.fromHex('#B89500'),
inputValidationWarningBackground: Color.fromHex('#352A05'),
inputValidationErrorBorder: Color.fromHex('#BE1100'),
inputValidationErrorBackground: Color.fromHex('#5A1D1D')
};

Contributor

ParkourKarthik commented Sep 5, 2018

@bpasero I've set the default values to null. I believe that is meant to be done when no actual defaults are to be set.

Note: I've not added any Validation Foreground colors to the below as there should not be defaults for Foreground.

const defaultOpts = {
inputBackground: Color.fromHex('#3C3C3C'),
inputForeground: Color.fromHex('#CCCCCC'),
inputValidationInfoBorder: Color.fromHex('#55AAFF'),
inputValidationInfoBackground: Color.fromHex('#063B49'),
inputValidationWarningBorder: Color.fromHex('#B89500'),
inputValidationWarningBackground: Color.fromHex('#352A05'),
inputValidationErrorBorder: Color.fromHex('#BE1100'),
inputValidationErrorBackground: Color.fromHex('#5A1D1D')
};

@bpasero

@ParkourKarthik much better. There is one remaining usage of inputValidationInfoBackground in messageController that can also have a foreground color from the theme.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 5, 2018

Member

Thanks 🍺

Member

bpasero commented Sep 5, 2018

Thanks 🍺

@bpasero bpasero merged commit 3053bc6 into Microsoft:master Sep 5, 2018

1 of 2 checks passed

VS Code in progress
Details
license/cla All CLA requirements met.
Details

@will-stone will-stone referenced this pull request Sep 16, 2018

Closed

Low contrast Git warning #25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment