Paste as plain text #24029
Paste as plain text #24029
Conversation
melvin-bot
bot
requested review from
mananjadhav
and removed request for
a team
|
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
I thought I signed the CLA after the issue popped #24029 (comment) #24029 (comment) |
desktop/main.js
Outdated
| id: 'pasteAsPlainText', | ||
| accelerator: PASTE_AS_PLAIN_TEXT_ACCELERATOR, | ||
| click: () => { | ||
| // Insert the plain text from the clipboard |
There was a problem hiding this comment.
This code is duplicated, let's create a method for this.
desktop/main.js
Outdated
| accelerator: PASTE_AS_PLAIN_TEXT_ACCELERATOR, | ||
| click: () => { | ||
| // Insert the plain text from the clipboard | ||
| const text = clipboard.readText(); |
There was a problem hiding this comment.
Generally if I copy an image and then try to Paste, macOS will default to disable the menu. Here I can see we always show the menu. Is there a way we can disable the menu in the Toolbar, and hide from the Context menu if there is no text to paste?
|
Raised some comments, @ygshbht. Can you please request a review once the changes are done? |
Co-authored-by: Manan <[email protected]>
|
@mananjadhav I have incorporataed the following changes:
Would you like to do Step 4 for Regarding the second part of step 4 - disable the menu in the Toolbar: Unfortunately there is no straightforward way of delecting when clipboard content changes or when the Toolbar Menu is going to show. This doesn't seem to work If you'd still like to go ahead with disabling these paste features from Menu bar if clipboard has image, we will have to use a combination of other events such as when the window regains focus, when user presses CtrlC CtrlV, when user Right Clicks, polling etc. This may still not provide 100% accuracy |
desktop/main.js
Outdated
| @@ -12,6 +12,8 @@ const CONST = require('../src/CONST').default; | |||
| const Localize = require('../src/libs/Localize'); | |||
|
|
|||
| const port = process.env.PORT || 8080; | |||
| const PASTE_AS_PLAIN_TEXT_ACCELERATOR = 'CmdOrCtrl+Shift+V'; | |||
There was a problem hiding this comment.
If we have more than one accelerators, then we should move this to CONST.
DESKTOP_SHORTCUT_ACCELERATOR: {
PASTE_AS_PLAIN_TEXT: ...,
PASTE_AND_MATCH_STYLE:...
}
| @@ -12,6 +12,7 @@ const CONST = require('../src/CONST').default; | |||
| const Localize = require('../src/libs/Localize'); | |||
|
|
|||
| const port = process.env.PORT || 8080; | |||
| const {DESKTOP_SHORTCUT_ACCELERATOR} = CONST; | |||
There was a problem hiding this comment.
I thought CONST is already imported on L11?
There was a problem hiding this comment.
Not importing here,just destructuing
| { | ||
| id: 'pasteAndMatchStyle', | ||
| role: 'pasteAndMatchStyle', | ||
| accelerator: DESKTOP_SHORTCUT_ACCELERATOR.PASTE_AND_MATCH_STYLE, |
There was a problem hiding this comment.
Why do we need to add the accelerator here?
There was a problem hiding this comment.
Apologies, as per my testing, shortcuts that are defined in the App Menu work, while the shortcuts defined in contextMenu dont. Their only purpose seems to show their value next to their name
There was a problem hiding this comment.
Thoretically, from what i understand, contextMenu shortcuts are meant to work only when contextMenu is open. However, practically, if you press any key, contextMenu is gone and the shortcut, thus, won't work
Reviewer Checklist
Screenshots/VideosWebNAMobile Web - ChromeNAMobile Web - SafariNADesktopdesktop-shortcut.moviOSNAAndroidNA |
There was a problem hiding this comment.
Thanks! @ygshbht can you update th ePR body to mention this hsould be tested only on desktop?
I am curious though, the context popup option will be only available in Desktop, right? it wint be in ios or web? that breaks our cross platform consistency, can we just ensure this works with the shortcut and not with the popup?
|
Thanks @mountiny. I have updated the PR. And you are correct in your observation. While we strive to maintain cross-platform consistency to the greatest extent possible, there are certain features that are platform-specific due to the nature of the platforms themselves and the norms established by other applications on the same platform. This is one such feature, and that's why we have platform-specific modules/files to handle these situations. The shortcuts have also been made functional globally, not just from the context menu. The inclusion in the context menu is a standard desktop app convention and serves to aid user visibility and discovery of the feature. Desktop applications often provide such menu options for features that are primarily accessible via shortcuts, distinct from browser behaviors. |
|
The issue's primarily about the lack of a 'Paste as Plain Text' option in the context menu, which I addressed in this PR. In doing so, I also ensured that its shortcut functions as expected. The changes I have made are specific to the desktop application and will not affect the web or mobile (iOS/Android) versions of the application. Context menus are a unique feature of desktop applications, and mobile platforms do not support them. In the web version, context menu behaviors are managed by the browser itself, not our application. By including the 'Paste as Plain Text' option in the desktop context menu, we are aligning our application with the conventions and user expectations of other desktop apps. This change will enhance our desktop application's user experience by making this feature more discoverable and accessible. |
|
I hope we can close this within 3 days. I'm happy to make any changes required |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.51-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.51-2 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.52-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.52-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Details
Fixed Issues
$ #23567
PROPOSAL: #23567 (comment)
Tests
This is a platform specific feature and needs to be tested only in macOS/ Desktop.
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
output.mp4
iOS
Android