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

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 fabiant3, ryalanms and SamBent 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

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

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

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 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

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

@miloush miloush miloush left review comments

@lindexi lindexi lindexi left review comments

@fabiant3 fabiant3 Awaiting requested review from fabiant3

@ryalanms ryalanms Awaiting requested review from ryalanms

@SamBent SamBent Awaiting requested review from SamBent

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