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

Update the messaging for the 'disconnected' dialog #116

Conversation

EdmondChuiHW
Copy link

@EdmondChuiHW EdmondChuiHW commented Sep 13, 2024

Summary

Quality of life improvement (no semantic changes in this PR).

Existing

Existing dialog can be dismissed by pressing the icon button.
Doing so will allow users to navigate around the DevTools while disconnected.

screenshot of the existing 'disconnected' dialog

Existing logs can be viewed, even though most features will stop working/appear flaky.
This has been addressed separately by highlighting the disconnected state in the UI #85:

screenshot of the new 'Reconnect DevTools' button

Problem

From our internal dogfooding, requests have been raised for the ability to preserve and view logs after disconnection.

This is already supported as discussed above, so it appears to be a discovery problem.

Note: this is a separate issue from some apps' logs being cleared from CDT upon disconnection.

Test plan

In this PR

  • Remove the icon button for closing the dialog
  • Add a new text button for dismissing the dialog
  • Add additional texts to explain each of the button's effect.
  • Arrange the buttons and texts into vertical stack.

screenshot of the new 'disconnected' dialog with the close button becoming a text button with explainations

Upstreaming plan

  • This commit should be sent as a patch to the upstream devtools-frontend repo. I've reviewed the contribution guide.
  • This commit is React Native-specific and cannot be upstreamed.

Comment on lines +30 to +47
/**
* @description Text in a dialog box to explain `DevTools` can still be used while disconnected.
*/
perserveState: 'Dismiss this dialog and continue using `DevTools` while disconnected.',
/**
* @description Text on a button to dismiss the dialog
*/
closeDialog: 'Dismiss dialog',
/**
* @description Text on a button to reconnect Devtools when remote debugging terminated.
* "Remote debugging" here means that DevTools on a PC is inspecting a website running on an actual mobile device
* (see https://developer.chrome.com/docs/devtools/remote-debugging/).
*/
reconnectDevtools: 'Reconnect `DevTools`',
/**
* @description Text in a dialog box to prompt for feedback if the disconnection is unexpected.
*/
sendFeedbackMessage: '[FB-only] Please send feedback if this disconnection is unexpected.',
Copy link
Author

Choose a reason for hiding this comment

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

Welcome any alternatives that's more concise!

Copy link

@huntie huntie left a comment

Choose a reason for hiding this comment

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

Love the intention here and agree with this change. Accepting, but I might loop back with another PR improving the visual treatment :)

/**
* @description Text in a dialog box to explain `DevTools` can still be used while disconnected.
*/
perserveState: 'Dismiss this dialog and continue using `DevTools` while disconnected.',
Copy link

Choose a reason for hiding this comment

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

Unnecessary backticks around "DevTools"

Copy link
Author

Choose a reason for hiding this comment

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

CDT has those in some existing strings. I suppose it gets swapped for something else if some build flags are specified? Let's do a pass on all of those and see if they fit our needs.

@@ -27,12 +27,24 @@ const UIStrings = {
* device back in a state where it can be inspected, before DevTools can reconnect to it.
*/
reconnectWhenReadyByReopening: 'Reconnect when ready by reopening DevTools.',
Copy link

Choose a reason for hiding this comment

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

I think we can also make this slightly more clear.

Suggested change
reconnectWhenReadyByReopening: 'Reconnect when ready by reopening DevTools.',
reconnectWhenReadyByReopening: 'Reconnect when ready (will reload DevTools).',

@EdmondChuiHW EdmondChuiHW merged commit 2c40d48 into facebookexperimental:main Sep 23, 2024
2 checks passed
@EdmondChuiHW EdmondChuiHW deleted the perserve-logs-when-disconnected branch September 23, 2024 12:21
@huntie huntie mentioned this pull request Oct 8, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants