Skip to content

Preserve format when copy/paste yaml file#4009

Merged
TheGostKasper merged 9 commits intomainfrom
3321
Sep 18, 2023
Merged

Preserve format when copy/paste yaml file#4009
TheGostKasper merged 9 commits intomainfrom
3321

Conversation

@TheGostKasper
Copy link
Copy Markdown
Contributor

Closes Preserve my formatting when I copy/paste excerpts from WGE UI yaml files

What changed?
Add a copy button to YamlView component

I updated CopyToClipboard component to accept different option ( icon , icon + text ) and added it to YamlView component

redirect3-2023-09-13_15.41.52.mp4
redirect3-2023-09-13_15.35.03.mp4

@TheGostKasper TheGostKasper added the area/ui Issues that require front-end work label Sep 13, 2023
@TheGostKasper
Copy link
Copy Markdown
Contributor Author

@mmoulian @LappleApple
I went for showing a copy Icon instead of icon+text, if you have any comments or you want the other option, please let me know .

@lasomethingsomething
Copy link
Copy Markdown
Contributor

Works for me @TheGostKasper -- thanks for jumping on this.

@opudrovs
Copy link
Copy Markdown
Contributor

opudrovs commented Sep 13, 2023

I've tested this branch and verified that copying YAML with the Copy button copies valid YAML and preserves the indents and does not insert extra space between the lines. ✨

But the acceptance criteria mention copying excerpts (so, probably portions of highlighted text):

As a platform dev, I would like to be able to preserve my line spacing and formatting when I copy-paste excerpts from the yaml file in the yaml tab, so that I don't insert errors by mistake or have to redo work.

And this comment from @jpellizzari mentions copying highlighted text but also mention adding a Copy button as an alternative (what this PR does):
weaveworks/weave-gitops-enterprise#3321 (comment)

When I highlight portions of YAML on the web page and copy it with Ctrl+C, this is what I see on paste:

Screenshot 2023-09-13 at 18 09 28

Indentation and space between lines are not preserved.

So, @LappleApple @jpellizzari could you please clarify if the acceptance criteria should be edited to only require copying the whole YAML with the Copy button or if the current PR should also include the "copy highlighted text and preserve indentation" functionality?

If only the former, I can approve this PR. If the latter is required too, more changes are needed. In principle, this issue could be split into two issues, because the first part is much easier to implement than copying highlighted text with preserving formatting.

@jpellizzari
Copy link
Copy Markdown
Contributor

@opudrovs

I think a Copy button is a good improvement, but we should be able to copy certain parts of the yaml file and have the formatting preserved.

So if it makes sense to fix the text-highlighting within this PR, then that is fine. If it's too big of a change, we can merge this and follow up with a PR that fixes the text highlighting.

@opudrovs
Copy link
Copy Markdown
Contributor

@jpellizzari OK, fine with me!

@lasomethingsomething
Copy link
Copy Markdown
Contributor

@jpellizzari @opudrovs Hi both, I didn't have time to discuss implementation details or add Acceptance Criteria before this got picked up via UI triage and over to Timberwolf. Great to see it moving forward. If you need me to create ACs let me know, but it looks like Jordan's got this?

@TheGostKasper
Copy link
Copy Markdown
Contributor Author

@opudrovs , the issue here is with showLineNumbers as it displays the content code in a flex way with custom style to the number section + code which require to add onCopy event globally to maintain the content to re-format it to the correct format (regex will be used , or customize the line style) --> Do we really need to do it!?

image

On the other hand removing showLineNumbers fron SyntaxHighlighter will display the content as
image

What options we have here ?

  • Remove showLineNumbers to preserve text format on Copy
  • Show a Copy icon to copy content
  • Customize style or use Reqex to preserve format for the copied lines
  • others... ?

The easiest way for now was to show the Icon .

@opudrovs
Copy link
Copy Markdown
Contributor

opudrovs commented Sep 14, 2023

@TheGostKasper hmm, interesting. Would it be possible to, maybe, display line numbers in some common margin (maybe with some before pseudo-elements) but position them absolutely relative to each line span and make them non-selectable?

Or if it's overcomplicated (probably it is), the decision depends on the Product's priorities: what is more important, displaying line numbers or being able to select portions of the YAML. Either option is fine with me.

The third option (using Regexes to clean up copied text) sounds interesting too and might the least troublesome if the copied text is consistent enough.

In principle, I would prefer to switch to another YAML/markdown/rich text component (maybe a third-party solution, whatever works) which supports selecting and copying code without line numbers. It should be such a standard task. 😅 But I am not sure how much time searching for the right component will take, like if it's worth it at all.

@TheGostKasper
Copy link
Copy Markdown
Contributor Author

@opudrovs changing lineProps to use textWrap instead of flexWrap solves it

@opudrovs
Copy link
Copy Markdown
Contributor

@TheGostKasper great, I'll re-test it now.

Can you please update the failing snapshot test?

Comment thread ui/components/YamlView.tsx Outdated
@opudrovs
Copy link
Copy Markdown
Contributor

opudrovs commented Sep 18, 2023

Re-tested it manually, works fine for me in Chrome. Great job! ✨

The only minor issue left is that when selecting and copying a portion of YAML in Firefox, blank lines are inserted between YAML lines.

This is what I see when copy-pasting several lines from Chrome (works as expected):

metadata:
  annotations:
    metadata.weave.works/CreatedBy: Value 4
    metadata.weave.works/Version: some version
    metadata.weave.works/created-by: Value 2
    metadata.weave.works/createdBy: Value 3
    metadata.weave.works/description: Value 1
    metadata.weave.works/html: <p><b>html</b></p>
    metadata.weave.works/link-to-google: https://google.com
    metadata.weave.works/multi-lines: |
      This is first line
      This is second line
  creationTimestamp: 2023-09-15T15:03:27Z
  finalizers:
    - finalizers.fluxcd.io
  generation: 1
  labels:
    kustomize.toolkit.fluxcd.io/name: flux-system
    kustomize.toolkit.fluxcd.io/namespace: flux-system

and this is what I see when copy-pasting from Firefox:

metadata:

  annotations:

    metadata.weave.works/CreatedBy: Value 4

    metadata.weave.works/Version: some version

    metadata.weave.works/created-by: Value 2

    metadata.weave.works/createdBy: Value 3

    metadata.weave.works/description: Value 1

    metadata.weave.works/html: <p><b>html</b></p>

    metadata.weave.works/link-to-google: https://google.com

    metadata.weave.works/multi-lines: |

      This is first line

      This is second line

  creationTimestamp: 2023-09-15T15:03:27Z

  finalizers:

    - finalizers.fluxcd.io

  generation: 1

  labels:

    kustomize.toolkit.fluxcd.io/name: flux-system

    kustomize.toolkit.fluxcd.io/namespace: flux-system

But I think it can be considered a Firefox issue.

Copy link
Copy Markdown
Contributor

@opudrovs opudrovs left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@TheGostKasper TheGostKasper merged commit 6f60f53 into main Sep 18, 2023
@TheGostKasper TheGostKasper deleted the 3321 branch September 18, 2023 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ui Issues that require front-end work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve my formatting when I copy/paste excerpts from WGE UI yaml files

4 participants