Use AceEditor for Query Snippets

[gabrieldutra]

What type of PR is this? (check all applicable)

• Other

Description

Following my idea on #3962 this makes AceEditor available on DynamicForm and applies it to Query Snippets.

I didn't find a simple way to align the validation mark (but didn't spend much time on it), for Query Snippets there's no need anyway.

Notes:

• It was necessary to create an almost useless Class component AceEditorInput only for the sake of avoiding this issue.
• Not sure why, but className didn't work with <AceEditor>

Related Tickets & Documents

--

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[kravets-levko]

It was necessary to create an almost useless Class component AceEditorInput only for the sake of avoiding this issue.

@gabrieldutra Can you please explain me what's the issue there? It looks really interesting that just wrapping a component fixes it 🤔

[gabrieldutra]

@gabrieldutra Can you please explain me what's the issue there? It looks really interesting that just wrapping a component fixes it

Sure, my assumption of the issue was that the AceEditor was changing the value internally, thus triggering Antd Form errors. By wrapping it in a Class Component I force the value to be the provided one, making it only changeable through the upper component, in this case Antd Form 🙂.

"I force the value to be the provided one" -> this seemed safe since AceEditor supports it and the effect is basically the same as having the AceEditor on any Class Component with its value on a state.

[ranbena]

Any way to hide the selected line and ghost cursor when editor field isn't focused?
It's really confusing cause it looks like it's already focused :/

[ranbena]

@gabrieldutra I can help with the validation mark alignment. What should I do to reproduce it?

[kravets-levko]

Sure, my assumption of the issue was that the AceEditor was changing the value internally, thus triggering Antd Form errors. By wrapping it in a Class Component I force the value to be the provided one, making it only changeable through the upper component, in this case Antd Form

I don't understand. Your wrapper passes all props to editor, and does not handle any changes, so if editor calls any callbacks/triggers events - your wrapper just passes them up. If editor modifies value in place - then wrapper also does not do anything: non-objects are always copied (therefore cannot be modified in place), and objects are passed by reference - wrapper does not create a copy, so if editor modifies an object - it should work exactly the same with and without a wrapper.

[gabrieldutra]

@ranbena I've pushed the furthest I got and enabled the feedback icons for testing purposes 🙂, I sort of gave up when I realized it was still not good for Scrollbars. Mind that disabling it for the Ace Editor is also an option (we do that for checkbox and file inputs).

@kravets-levko to be honest I don't 100% understand why this happened and why this works, I actually saw the errors and figured that wrapping it would somehow "isolate" the Editor in a situation where the component value wouldn't get updated, as you mentioned.

It's probably something to do with the way ReactAce was implemented that conflicts with AntdForm. From a quick look on their code they use a ref for the editor and set/get value from it. I guess with a ref it's possible to change the component's value.

[kravets-levko]

From a quick look on their code they use a ref for the editor and set/get value from it. I guess with a ref it's possible to change the component's value.

Yeah, that's possible. Probably AntdForm takes a ref to each element and expects to see some Ant control there, but for ReactAce it gets something else but tries to use it. Wrapping it with a component isolated ReactAce's ref from Ant. It's interesting why functional component does not work in this case, but I think there may be some difference in how React passes ref for class-based and functional components. Can you do an experiment to confirm this theory? Rewrite wrapper component in this way:

import React, { useImperativeHandle } from 'react';

function AceEditorInput(props, ref) {
  useImperativeHandle(ref, () => null); // expose `null` as this component's ref
  return (
    // all the same as now
  );
}

[gabrieldutra]

The issue of this component as a Functional Component is this (Ant Control needs the component Ref):

The useImperativeHandle actually breaks it 😅. Surprisingly, 68ae87d works.

[ranbena]

An alternative that I think makes better sense #3974

[gabrieldutra]

Should we keep the icons for the Query Snippets context or...

Suggested change

feedbackIcons

?

[ranbena]

It's a really nice and fun UX, but not a must. I have no strong opinion.