allow to disable DevMenu using new method isSecondaryClickToShowDevMenuEnabled #528

Simek · 2020-08-03T23:41:10Z

Please select one of the following

I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
I am making a fix / change for the macOS implementation of react-native
I am making a change required for Microsoft usage of react-native

Summary

This change allows developers to disable the DevMenu (context menu in macOS case) by ~~reusing currently exported isShakeToShowDevMenuEnabled available on iOS~~ addding new setIsSecondaryClickToShowDevMenuEnabled method to the DevSettings:

NativeModules.DevSettings.setIsSecondaryClickToShowDevMenuEnabled(false);

This also improves quite a bit the development process when using Contextual Menus (aka ActionSheetIOS).

Of course I can implement separate, exclusive for macOS, flag if you think it's a better approach.

Also adding [devSettings isDevModeEnabled] like in showOnShake fixed for me DevMenu appearing on the Release build.

Changelog

[macOS] [Added] - add ability to disable DevMenu based on new setIsSecondaryClickToShowDevMenuEnabled method in DevSettings
[macOS] [Fixed] - disable DevMenu when isDevModeEnabled returns false

Test Plan

I have tested the change inside my workbench/sample app, which source code is available here.

Microsoft Reviewers: Open in CodeFlow

tom-un · 2020-08-10T06:25:22Z

Hi @Simek : Just FYI I've been on vacation and will be on vacation for another week. Thanks for the contribution. This is great, but I do think it should have its own DevSetting instead of reusing isShakeToShowDevMenuEnabled. Perhaps isSecondaryClickToShowDevMenuEnabled.

Simek · 2020-08-17T15:56:06Z

@tom-un Thank you for the review Tom, I have updated the PR according to the request - there is an macOS exclusive method named isSecondaryClickToShowDevMenuEnabled now and I have tried to add correct macOS merge comments too.

Let me know if it's okay or should I update or change anything else. 🙂

alloy · 2020-08-18T18:01:27Z

@tom-un For my understanding, why would it be preferable to add another setting instead of perhaps renaming the existing one, or adding a new generic one and deprecate the existing ‘shake’ one?

tom-un · 2020-08-19T08:24:35Z

@tom-un For my understanding, why would it be preferable to add another setting instead of perhaps renaming the existing one, or adding a new generic one and deprecate the existing ‘shake’ one?

I just feel its cleaner to have prop names that mean what they say. Also there could be shake gesture on mac in the future (there already is via Catalyst) or there could be a secondary click gesture in a future iOS.

tom-un · 2020-08-19T08:27:29Z

React/DevSupport/RCTDevMenu.m

-      desc = NSStringFromClass([_bridge class]);
-    }
-    NSString *title = [NSString stringWithFormat:@"React Native: Development\n(%@)", desc];
+  RCTDevSettings *devSettings = _bridge.devSettings; // TODO(OSS Candidate ISS#2710739)


nit: this comment is not necessary: this whole block of code is already enclosed in #if TARGET_OS_OSX // [TODO(macOS ISS#2323203) ... #else // ]TODO(macOS ISS#2323203)

This comment refers to other issue than macOS fork comment, the line has been copied from showOnShake:

react-native-macos/React/DevSupport/RCTDevMenu.m

Line 184 in 5639653

RCTDevSettings *devSettings = _bridge.devSettings; // TODO(OSS Candidate ISS#2710739)

alloy · 2020-08-19T10:00:10Z

I just feel its cleaner to have prop names that mean what they say.

Totally agreed, but I also like to have less API surface for ease of discovery. However…

Also there could be shake gesture on mac in the future (there already is via Catalyst) or there could be a secondary click gesture in a future iOS.

…I didn’t consider the real possibility of having multiple ways to to open the menu. Although that thinking also makes me wonder if it shouldn’t be an enum?

Anyways, something to consider longer term, I don’t feel strong about it for this PR.

Simek · 2020-08-20T19:30:20Z

Should I update this PR to 0.62 files structure or should I target the 0.61-stable branch instead?

alloy · 2020-08-24T11:30:40Z

Update to the 0.62 structure, which is now in master, please 🙏

Simek · 2020-08-24T12:56:17Z

Update to the 0.62 structure, which is now in master, please 🙏

Not a problem @alloy, I will work on the changes this week 🙂

Simek · 2020-09-01T14:32:17Z

@alloy Should I force push the branch again to rerun the ci/circleci: test_js? It looks like this failure is not related to the changes in this PR.

tom-un · 2020-09-01T15:31:10Z

@alloy Should I force push the branch again to rerun the ci/circleci: test_js? It looks like this failure is not related to the changes in this PR.

I tried forcing it to re-run and it failed again. I'm not sure why. As a branch of the upstream react-native we get these circleci tests too. They're mostly redundant as we do all the same jest, lint, flow and format-check tests in Azure DevOps. I'll force merge this PR and watch for failures in other PRs.

Thanks!

HeyImChris · 2020-09-01T15:47:39Z

@alloy Should I force push the branch again to rerun the ci/circleci: test_js? It looks like this failure is not related to the changes in this PR.

I tried forcing it to re-run and it failed again. I'm not sure why. As a branch of the upstream react-native we get these circleci tests too. They're mostly redundant as we do all the same jest, lint, flow and format-check tests in Azure DevOps. I'll force merge this PR and watch for failures in other PRs.

Thanks!

Late here but they're really flaky. I wouldn't go off of the circle CI tests at all as long as our ADO tests pass.

alloy · 2020-09-01T15:56:31Z

@Simek Thanks again for your work! 🙌

Simek requested a review from tom-un as a code owner August 3, 2020 23:41

Simek changed the title ~~allow to disable DevMenu using isShakeToShowDevMenuEnabled~~ allow to disable DevMenu using new method isSecondaryClickToShowDevMenuEnabled Aug 17, 2020

acoates-ms mentioned this pull request Aug 17, 2020

Question: Is it possible to disable Developer Menu in Release #545

Closed

tom-un approved these changes Aug 19, 2020

View reviewed changes

Simek added 3 commits August 31, 2020 17:21

allow to disable DevMenu using isShakeToShowDevMenuEnabled

c898f82

review changes: add macOS exclusive method

2f56172

fix structure after 0.62 merge

cc44550

Simek force-pushed the allow-disable-devmenu branch from 5639653 to cc44550 Compare August 31, 2020 15:56

isDevModeEnable is no longer required in 0.62

ec3d997

tom-un merged commit a9073e3 into microsoft:master Sep 1, 2020

kelset mentioned this pull request Jun 8, 2022

android-patches status round up #1192

Closed

allow to disable DevMenu using new method isSecondaryClickToShowDevMenuEnabled #528

allow to disable DevMenu using new method isSecondaryClickToShowDevMenuEnabled #528

Uh oh!

Conversation

Simek commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please select one of the following

Summary

Changelog

Test Plan

Microsoft Reviewers: Open in CodeFlow

Uh oh!

tom-un commented Aug 10, 2020

Uh oh!

Simek commented Aug 17, 2020

Uh oh!

alloy commented Aug 18, 2020

Uh oh!

tom-un commented Aug 19, 2020

Uh oh!

tom-un Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

Simek Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alloy commented Aug 19, 2020

Uh oh!

Simek commented Aug 20, 2020

Uh oh!

alloy commented Aug 24, 2020

Uh oh!

Simek commented Aug 24, 2020

Uh oh!

Simek commented Sep 1, 2020

Uh oh!

tom-un commented Sep 1, 2020

Uh oh!

HeyImChris commented Sep 1, 2020

Uh oh!

alloy commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Simek commented Aug 3, 2020 •

edited

Loading

Simek Aug 19, 2020 •

edited

Loading