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

Feat/4750 Make code type fields copyable #5395

Merged
merged 6 commits into from
Feb 21, 2024
Merged

Feat/4750 Make code type fields copyable #5395

merged 6 commits into from
Feb 21, 2024

Conversation

elzody
Copy link
Contributor

@elzody elzody commented Feb 19, 2024

📝 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

grafik

Collectives edit mode The button makes way for code block options when in Edit mode

grafik

Copy toast text This text is now displayed when code from a code block or a link from a link bubble view is copied.

grafik

🚧 TODO

  • ...

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

Copy link
Member

@juliushaertl juliushaertl left a 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.

@@ -104,6 +125,9 @@ export default {
}
},
computed: {
hasCode() {
return this.node.textContent
Copy link
Member

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?

Suggested change
return this.node.textContent
return this.node?.textContent

Copy link
Contributor Author

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 :)

@juliushaertl juliushaertl added enhancement New feature or request 2. developing labels Feb 19, 2024
@juliushaertl juliushaertl added this to the Nextcloud 29 milestone Feb 19, 2024
Copy link
Collaborator

@max-nextcloud max-nextcloud left a 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>`,
Copy link
Collaborator

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.

Copy link
Member

@juliushaertl juliushaertl left a 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

// 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()
Copy link
Member

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

@juliushaertl juliushaertl merged commit 53858a1 into main Feb 21, 2024
58 checks passed
@juliushaertl juliushaertl deleted the feat/4750 branch February 21, 2024 07:34
@juliushaertl
Copy link
Member

/backport to stable28

@juliushaertl
Copy link
Member

Backporting even though it is an enhancement as it helps users to work around #5331

@elzody elzody linked an issue Apr 8, 2024 that may be closed by this pull request
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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!

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.

Can no longer copy content from code blocks Make code type fields copyable
3 participants