Bug 2167608: highlight parts #1141

upalatucci · 2023-03-14T10:06:07Z

📝 Description

Highlight editor parts in the LIVE YAML

🎥 Demo

editor-2023-03-14_12.06.57.mp4

openshift-ci · 2023-03-14T10:36:25Z

@upalatucci: This pull request references Bugzilla bug 2167608, which is invalid:

expected the bug to target the "4.14.0" release, but it targets "4.13.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2167608: highlight parts

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

upalatucci · 2023-03-14T10:55:26Z

/retest

src/utils/components/SidebarEditor/useEditorHighlighter.ts

src/utils/components/SidebarEditor/utils.ts

metalice · 2023-03-14T11:35:10Z

src/utils/components/SidebarEditor/utils.ts

+      (line, index) => index > startPropertyRange && line.match(/^[A-z]+:/g),
+    );
+
+    if (endPropertyRange === -1) endPropertyRange = rangeLines.length;


can be removed?

Considering the previous suggestion i can do something different but in general i need this.
Here i understand that the range is finished because i find another property using the regex /^[A-z]+:/g.

So for example if i want to select the property spec i know the start because it starts with spec: and i know the end because i encounter another property at the same level status: with his conditions.

In that case i know that the spec range is finished.

But if I want to select the property status, i know that the range is finished because there isn't another property after. So here endPropertyRange would be -1. the findIndex function does not find anything else after.

So at that point, i know that the end is equivalent to the last line

Ok cool. But what's wrong in the code I suggested?

Oh I see you took part of it

src/views/virtualmachines/details/tabs/details/VirtualMachineDetailsPage.tsx

upalatucci · 2023-03-14T12:50:42Z

/retest

openshift-ci · 2023-03-14T12:59:58Z

@upalatucci: This pull request references Bugzilla bug 2167608, which is invalid:

expected the bug to target the "4.14.0" release, but it targets "4.13.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 2167608: highlight parts

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

upalatucci · 2023-03-14T13:55:44Z

/retest

upalatucci · 2023-03-15T14:07:51Z

/retest

hstastna

The code looks good, +1 for the comments in getLineFromPath.

When I tested the code a while ago, the editor did highlight the relevant lines nicely, however when I changed something in the VM details, e.g. changed Description field (by clicking on that field and adding text to the modal and saving), the yaml updated, but didn't highlight the lines again, got stuck. I had to turn the switcher off and on again to make it displayed highlighting the lines again.
Also the description info didn't occur in the yaml the expected way - at least for me - I mean it didn't occurred there easily visible, but hidden, I had to click on > to expand the annotations in the editor to reveal that part of the yaml - and that was a bit annoying to me, especially when I edited info belonging to that part of the yaml. Not sure if this is the behavior we expect. Additionally, the description was not highlighted at all, not before, nor after editing it - that's not what I'd expect for sure.

To sum up, IMO we should highlight all the relevant lines (e.g. also Description, when being on VM Details tab), not only most of them. PTAL into that a bit. Also I think we should highlight lines also after some editing (not only before) :) And If we should display some yaml parts expanded or not - that's what I'm not sure about. Depends on the design/what user prefer. Otherwise great work, Ugo! ❇️

src/utils/resources/vm/utils/constants.ts

src/utils/components/SidebarEditor/SidebarEditor.tsx

metalice · 2023-03-16T11:07:38Z

src/utils/components/SidebarEditor/useEditorHighlighter.ts

+  showEditor: boolean,
+) => {
+  const [editor, setEditor] = useState<YAMLEditorRef['editor']>();
+  const isHighlighed = useRef(false);


why do we need it? if you will use showEditor as your dependency at line 36 we will get same effect?

This is because the editor would trigger the useEffect everytime editableYAML is modified.
So every time something change in the editor.

For example, while the user types something, the editor continues to unfold and fold parts for the code

src/utils/components/SidebarEditor/utils.ts

upalatucci · 2023-03-16T13:33:15Z

@hstastna i've addressed your issue and added metadata.annotations to the selection in the details tab.
Unfortunately, if you change something, the highlight goes away because its not really an highlight, its more like a selection.
But we can find other solutions after this pr if you want

hstastna · 2023-03-16T14:45:24Z

@hstastna i've addressed your issue and added metadata.annotations to the selection in the details tab. Unfortunately, if you change something, the highlight goes away because its not really an highlight, its more like a selection. But we can find other solutions after this pr if you want

I am not sure if this actual version of the solution wouldn't add just more confusion to the users, if it worked only before they change anything. Maybe we could simulate that my "quick fix" of turning off and on the YAML switcher, so the fields would be selected/highlighted again? Or to reload/rerender the whole editor somehow after the user changes something?

metalice · 2023-03-16T15:05:12Z

ok by me , @hstastna when you are satisfied with this pr give it a go :)

hstastna · 2023-03-16T15:14:01Z

I am not sure if this actual version of the solution wouldn't add just more confusion to the users, if it worked only before they change anything. Maybe we could simulate that my "quick fix" of turning off and on the YAML switcher, so the fields would be selected/highlighted again? Or to reload/rerender the whole editor somehow after the user changes something?

I'd give this mentioned by me above a try at least, before I give this PR a go. WDYT, @upalatucci ?

upalatucci · 2023-03-17T14:54:46Z

/retest

upalatucci · 2023-03-17T16:06:50Z

Guys i don't know. At this point, I'll contact Yifat and see what she thinks. I'll share with her what i think we can do with the editor, and let her figure out what we should do with that.

I can't re-highlight the editor when something gets edited because its a live editor, so you can edit things from the editor and also from the form. For example in the Environment tab, you can write down envs values in the yaml and also in the form and you'll see in real time the result on the other side. So i don't think we want the yaml to highlight at every char pressed.

hstastna · 2023-03-17T17:23:10Z

Guys i don't know. At this point, I'll contact Yifat and see what she thinks. I'll share with her what i think we can do with the editor, and let her figure out what we should do with that.

I can't re-highlight the editor when something gets edited because its a live editor, so you can edit things from the editor and also from the form. For example in the Environment tab, you can write down envs values in the yaml and also in the form and you'll see in real time the result on the other side. So i don't think we want the yaml to highlight at every char pressed.

Ok, great, so let's ask Yifat and we'll see. If she's gonna be okay with what we actually have, let's merge this. Just make sure with her. Thanks! :)

upalatucci · 2023-03-27T14:35:01Z

/retest

upalatucci · 2023-03-28T13:28:29Z

/retest

metalice · 2023-03-29T11:58:39Z

/lgtm

openshift-ci · 2023-03-29T11:58:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: metalice, upalatucci

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [metalice,upalatucci]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-03-29T13:28:50Z

@upalatucci: All pull requests linked via external trackers have merged:

Bugzilla bug 2167608 has been moved to the MODIFIED state.

In response to this:

Bug 2167608: highlight parts

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

upalatucci force-pushed the editor-highlight branch from dc98322 to e3d50b2 Compare March 14, 2023 10:06

openshift-ci bot requested review from metalice and yaacov March 14, 2023 10:06

openshift-ci bot added the approved This issue is something we want to fix label Mar 14, 2023

upalatucci force-pushed the editor-highlight branch 2 times, most recently from f91fcd8 to b42372d Compare March 14, 2023 10:31

upalatucci changed the title ~~feat: highlight parts~~ Bug 2167608: highlight parts Mar 14, 2023

openshift-ci bot added bugzilla/severity-medium bugzilla/invalid-bug labels Mar 14, 2023

tnisan removed the bugzilla/invalid-bug label Mar 14, 2023

upalatucci force-pushed the editor-highlight branch from b42372d to 878ae32 Compare March 14, 2023 11:08

metalice reviewed Mar 14, 2023

View reviewed changes

src/utils/components/SidebarEditor/useEditorHighlighter.ts Show resolved Hide resolved

metalice reviewed Mar 14, 2023

View reviewed changes

src/utils/components/SidebarEditor/utils.ts Outdated Show resolved Hide resolved

metalice reviewed Mar 14, 2023

View reviewed changes

src/views/virtualmachines/details/tabs/details/VirtualMachineDetailsPage.tsx Outdated Show resolved Hide resolved

openshift-ci bot added the bugzilla/invalid-bug label Mar 14, 2023

upalatucci force-pushed the editor-highlight branch from 878ae32 to 3a5eff3 Compare March 14, 2023 13:23

metalice added bugzilla/valid-bug and removed bugzilla/invalid-bug labels Mar 14, 2023

upalatucci force-pushed the editor-highlight branch 2 times, most recently from 6b5fe36 to 80d91c2 Compare March 15, 2023 13:41

openshift-merge-robot added the needs-rebase label Mar 15, 2023

upalatucci force-pushed the editor-highlight branch from 80d91c2 to ff86e16 Compare March 15, 2023 13:42

openshift-merge-robot removed the needs-rebase label Mar 15, 2023

upalatucci force-pushed the editor-highlight branch from ff86e16 to 8f2754e Compare March 15, 2023 13:47

hstastna reviewed Mar 15, 2023

View reviewed changes

src/utils/resources/vm/utils/constants.ts Outdated Show resolved Hide resolved

src/utils/components/SidebarEditor/SidebarEditor.tsx Outdated Show resolved Hide resolved

metalice reviewed Mar 16, 2023

View reviewed changes

src/utils/components/SidebarEditor/utils.ts Outdated Show resolved Hide resolved

metalice reviewed Mar 16, 2023

View reviewed changes

src/utils/components/SidebarEditor/utils.ts Outdated Show resolved Hide resolved

upalatucci force-pushed the editor-highlight branch from 8f2754e to 5debca7 Compare March 16, 2023 13:34

openshift-merge-robot added the needs-rebase label Mar 26, 2023

feat: highlight parts

d5ab3e0

upalatucci force-pushed the editor-highlight branch from 5debca7 to d5ab3e0 Compare March 27, 2023 13:57

openshift-merge-robot removed the needs-rebase label Mar 27, 2023

openshift-ci bot assigned metalice Mar 29, 2023

openshift-ci bot added the lgtm Passed code review, ready for merge label Mar 29, 2023

openshift-merge-robot merged commit 223de4f into kubevirt-ui:main Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 2167608: highlight parts #1141

Bug 2167608: highlight parts #1141

upalatucci commented Mar 14, 2023 •

edited

Loading

openshift-ci bot commented Mar 14, 2023

upalatucci commented Mar 14, 2023

metalice Mar 14, 2023

upalatucci Mar 14, 2023

metalice Mar 14, 2023

metalice Mar 14, 2023

upalatucci commented Mar 14, 2023

openshift-ci bot commented Mar 14, 2023

upalatucci commented Mar 14, 2023

upalatucci commented Mar 15, 2023

hstastna left a comment •

edited

Loading

metalice Mar 16, 2023

upalatucci Mar 16, 2023 •

edited

Loading

upalatucci commented Mar 16, 2023

hstastna commented Mar 16, 2023 •

edited

Loading

metalice commented Mar 16, 2023

hstastna commented Mar 16, 2023

upalatucci commented Mar 17, 2023

upalatucci commented Mar 17, 2023 •

edited

Loading

hstastna commented Mar 17, 2023

upalatucci commented Mar 27, 2023

upalatucci commented Mar 28, 2023

metalice commented Mar 29, 2023

openshift-ci bot commented Mar 29, 2023

openshift-ci bot commented Mar 29, 2023

Bug 2167608: highlight parts #1141

Bug 2167608: highlight parts #1141

Conversation

upalatucci commented Mar 14, 2023 • edited Loading

📝 Description

🎥 Demo

openshift-ci bot commented Mar 14, 2023

upalatucci commented Mar 14, 2023

metalice Mar 14, 2023

Choose a reason for hiding this comment

upalatucci Mar 14, 2023

Choose a reason for hiding this comment

metalice Mar 14, 2023

Choose a reason for hiding this comment

metalice Mar 14, 2023

Choose a reason for hiding this comment

upalatucci commented Mar 14, 2023

openshift-ci bot commented Mar 14, 2023

upalatucci commented Mar 14, 2023

upalatucci commented Mar 15, 2023

hstastna left a comment • edited Loading

Choose a reason for hiding this comment

metalice Mar 16, 2023

Choose a reason for hiding this comment

upalatucci Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

upalatucci commented Mar 16, 2023

hstastna commented Mar 16, 2023 • edited Loading

metalice commented Mar 16, 2023

hstastna commented Mar 16, 2023

upalatucci commented Mar 17, 2023

upalatucci commented Mar 17, 2023 • edited Loading

hstastna commented Mar 17, 2023

upalatucci commented Mar 27, 2023

upalatucci commented Mar 28, 2023

metalice commented Mar 29, 2023

openshift-ci bot commented Mar 29, 2023

openshift-ci bot commented Mar 29, 2023

upalatucci commented Mar 14, 2023 •

edited

Loading

hstastna left a comment •

edited

Loading

upalatucci Mar 16, 2023 •

edited

Loading

hstastna commented Mar 16, 2023 •

edited

Loading

upalatucci commented Mar 17, 2023 •

edited

Loading