-
Notifications
You must be signed in to change notification settings - Fork 91
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
Feat/4750 Make code type fields copyable #5395
Conversation
Signed-off-by: Elizabeth Danzberger <[email protected]>
Signed-off-by: Elizabeth Danzberger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, works like a charm 👍
Just a small nitpick, and the cypress tests seem to need an adjustment as we have two buttons now. Otherwise good to get in from my side.
src/nodes/CodeBlockView.vue
Outdated
@@ -104,6 +125,9 @@ export default { | |||
} | |||
}, | |||
computed: { | |||
hasCode() { | |||
return this.node.textContent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is safeguarded further down, do we need this here as well?
return this.node.textContent | |
return this.node?.textContent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial thought process was that, since it's just checking to see if there is any text content in the node (the code block has something written in it), it wouldn't matter if we used the optional chaining ?.
or not, but I think for consistency you're right, it should be added there as well. As for the cypress tests, I was just about to look into those and give it a fix. Thank you :)
Signed-off-by: Elizabeth Danzberger <[email protected]>
Signed-off-by: Elizabeth Danzberger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the well prepared PR 🎉 - looks good to me.
I have one small comment fyi and will approve any way as I think this is good to go as is. The things Julius brought up have been addressed as far as i can tell.
} catch (error) { | ||
this.copySuccess = false | ||
this.copied = true | ||
showError( | ||
`<div>${t('collectives', 'Could not copy link to the clipboard:')}</div><div>${url}</div>`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly the url was in here on purpose. The idea was that if your browser cannot copy the url for you its still displaying it so you can copy it yourself.
I'm not sure how often people would need to do that. I remember that back in the days I could not copy share links because of my browser configuration and did not find a way around that.
However in this case I think it's different. I could open the link in a tab and copy the link from there - or I right click the link and select 'copy link' from the context menu.
So I'm fine with removing the url - just wanted to explain the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment on the test selector, feel free to fix and merge then
cypress/e2e/nodes/CodeBlock.spec.js
Outdated
// Remove language | ||
cy.getContent().find('.code-block').eq(1).find('.view-switch button').click() | ||
cy.getContent().find('.code-block').eq(1).find('.view-switch div div button').click() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a cleaner test case we should actually add unique identifiers to both NcActionButtons and then select them by identifier
https://docs.cypress.io/guides/references/best-practices#Selecting-Elements
Signed-off-by: Elizabeth Danzberger <[email protected]>
Signed-off-by: Elizabeth Danzberger <[email protected]>
/backport to stable28 |
Backporting even though it is an enhancement as it helps users to work around #5331 |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
📝 Summary
This change introduces a code copy button on code blocks in applications that use Text (Notes, Collectives, Tables, ...). The button utilizes a method which was already present in the application for copying links from the link bubble view (src/mixins/CopyToClipboard.js), and makes changes to this function in order to make it more generalized so that it may be used for both purposes. The LinkBubbleView.vue component remains unaffected and should still work normally.
The button only appears when the code block is actually populated with text/code, and in Collectives it will appear in both Preview and Edit mode. I figured even those who do not have edit access may still need/want to copy the content of the code block.
🖼️ Screenshots
Collectives preview mode
Collectives edit mode
The button makes way for code block options when in Edit modeCopy toast text
This text is now displayed when code from a code block or a link from a link bubble view is copied.🚧 TODO
🏁 Checklist
npm run lint
/npm run stylelint
/composer run cs:check
)