Skip to content

Implemented the RecognizesAccessKey property in the label and its def… #3878

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Implemented the RecognizesAccessKey property in the label and its def… #3878

wants to merge 1 commit into from

Conversation

@Nils-Berghs
Copy link

@Nils-Berghs Nils-Berghs commented Dec 2, 2020

Implemented the RecognizesAccessKey property in the label and its default styles #3877

@Nils-Berghs Nils-Berghs requested a review from a team as a code owner December 2, 2020 19:36
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Dec 2, 2020
@ghost ghost requested review from SamBent, fabiant3 and ryalanms December 2, 2020 19:37
@dnfadmin
Copy link

dnfadmin commented Dec 2, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

lindexi
lindexi reviewed Dec 3, 2020
View reviewed changes
src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/Label.cs
"RecognizesAccessKey",
typeof(bool),
typeof(Label),
new FrameworkPropertyMetadata(true));
Copy link
Member

@lindexi lindexi Dec 3, 2020

Choose a reason for hiding this comment

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

I do not think the default value should be true.

Copy link
Author

@Nils-Berghs Nils-Berghs Dec 3, 2020

Choose a reason for hiding this comment

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

I put it to true for backwards compatibility. Am I missing something?

Copy link
Contributor

@miloush miloush Dec 3, 2020

Choose a reason for hiding this comment

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

Correct, not being it true by default would be a serious breaking change.

Any reason for not using ContentPresenter.RecognizesAccessKeyProperty.AddOwner instead of declaring a new one?

@ryalanms
Copy link
Member

ryalanms commented Jan 4, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jan 4, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@ryalanms
Copy link
Member

ryalanms commented Jan 7, 2021

Hi @Nils-Berghs

This is an API change that will require approval from at least one other WPF developer. The build is failing due to our API compatibility checks.

/cc @SamBent

@ryalanms ryalanms added Priority:1 Work that is critical for the release, but we could probably ship without Queue for test pass and removed Queue for test pass labels Jan 7, 2021
@miloush
Copy link
Contributor

miloush commented Jan 10, 2021

As per discussion in #3877 I don't think should be merged as it defeats the purpose of the Label control.

@ryalanms
Copy link
Member

ryalanms commented Feb 10, 2021

Adding @MikeHillberg for API review...

Base automatically changed from master to main March 17, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabiant3 fabiant3 Awaiting requested review from fabiant3

@ryalanms ryalanms Awaiting requested review from ryalanms

@SamBent SamBent Awaiting requested review from SamBent

2 more reviewers

@miloush miloush miloush left review comments

@lindexi lindexi lindexi left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

API Review Requested PR metadata: Label to tag PRs, to facilitate with triage Priority:1 Work that is critical for the release, but we could probably ship without

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Nils-Berghs @dnfadmin @ryalanms @miloush @lindexi @WhyDoUserNamesNeedToBeUnique