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

Change one BasicMouseHandler properties from private to protected #161

Merged
merged 3 commits into from Mar 31, 2021

Conversation

ibdafna
Copy link
Member

@ibdafna ibdafna commented Mar 25, 2021

Signed-off-by: Itay Dafna i.b.dafna@gmail.com

Whilst the BasicMouseHandler class can be extended, it has two private properties, making extensions tricky to implement. This PR makes the the _pressData property protected instead of private.

Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna
Copy link
Member Author

ibdafna commented Mar 25, 2021

@jasongrout @blink1073

Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna ibdafna changed the title Change two BasicMouseHandler properties from private to protected Change one BasicMouseHandler properties from private to protected Mar 31, 2021
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks! The windows CI failure is unrelated. Once we get #165 merged I'll cut a release with this.

@blink1073 blink1073 merged commit 66efb84 into jupyterlab:master Mar 31, 2021
@ibdafna ibdafna deleted the private_to_protected branch March 31, 2021 18:33
get pressData(): PressData.PressData | null {
return this._pressData;
}

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this mean that this property will not be available on the public API anymore ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah agreed. Can this be added back?

Copy link
Member

Choose a reason for hiding this comment

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

Anyone able to make a PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll submit one

Copy link
Member Author

Choose a reason for hiding this comment

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

@blink1073 to revert this, I'll need to restore the variable name of pressData back to _pressData as the get pressData() and pressData attribute have duplicate identifiers. Unless you have other suggestions?

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 in #167

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2022
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.

None yet

4 participants