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

Use the Monaco Editor for the JSON Edit component #4051

Closed
Viicos opened this issue Mar 25, 2024 · 2 comments · Fixed by open-formulieren/formio-builder#153, open-formulieren/formio-builder#158 or #4417

Comments

@Viicos
Copy link
Contributor

Viicos commented Mar 25, 2024

The admin currently uses a basic editor for JSON content. Using the Monaco Editor will greatly improve dealing with JSON (folding, auto-closing brackets/quotes, etc).

There are two implementations of the editor in React:

https://github.com/suren-atoyan/monaco-react
https://github.com/react-monaco-editor/react-monaco-editor

The second one requires some webpack plugin, and I couldn't make it work. The first one does seem to work fine, so it should be used.

@Viicos Viicos added topic: admin triage Issue needs to be validated. Remove this label if the issue considered valid. labels Mar 25, 2024
@joeribekker joeribekker added enhancement and removed triage Issue needs to be validated. Remove this label if the issue considered valid. labels Mar 25, 2024
@joeribekker
Copy link
Contributor

Refinement: We don't see any downsides (slightly bigger download) and only improvements so, we want to pick this up. We do need however first a list of places where it is used so it can also be properly tested.

@joeribekker joeribekker added this to the Release 2.7.0 milestone Mar 25, 2024
@joeribekker joeribekker assigned Viicos and unassigned vaszig Apr 23, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 1, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 1, 2024
'JSON' and 'JSON Edit' have been merged in a single view
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 1, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 1, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 8, 2024
This was already an issue before: the JSON component
wasn't wrapped in a Formik component.
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 14, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 14, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 14, 2024
The Monaco Editor is almost impossible to test with testing-library
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 14, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
The component doesn't play well at all with SB
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
The component doesn't play well at all with SB
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
The component doesn't play well at all with SB
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
The component doesn't play well at all with SB
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
The test only tests the mock implementation, which is useless
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
There's two Formik contexts, one for the component JSON
and one for the preview. Also fix an existing bug
where context wasn't provided in stories
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
'JSON' and 'JSON Edit' have been merged in a single view
Viicos added a commit to open-formulieren/formio-builder that referenced this issue May 30, 2024
sergei-maertens pushed a commit that referenced this issue Jul 10, 2024
sergei-maertens pushed a commit that referenced this issue Jul 10, 2024
sergei-maertens added a commit that referenced this issue Jul 12, 2024
It takes any json, as primitives or arrays are also valid JSON logic.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Upgraded the builder to a version that uses the same
monaco-json-editor version to prevent different library
versions being included in the build.

This adds the explicit dependency on the monaco editor,
instead of relying on it being provided through
formio-builder.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
These components now use the underlying monaco-json-editor to edit
and/or preview the JSON data being manipulated. This fixes the annoying
autoformatting-while-typing and adds syntax highlighting, plus support
for dark/light theme.

Co-authored-by: @Viicos
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Sizing is based on number of rows/cols desired and set through a custom
CSS property on the respective components.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
* Moved around the position of widgets a bit, and declutter
* Instead of a toggle link, there's now a toggle icon
* Instead of expand/collapse, replace the editor UI with the Json widget
  and vice versa on toggle
* Ensure that icons have accessible labels when a title is provided
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Added a helper to put (JSON) expressions in the editors so that the
user interaction can be simulated. For speed reasons, it uses copy and
paste. Solution is taken from microsoft/playwright#14126
sergei-maertens added a commit that referenced this issue Jul 12, 2024
It leads to duplicate labels where the icon is used as a tooltip,
instead we can provide the prop/attribute in places where it's actually
waranted.

This refactors the toggle icon so that the message, behaviour and a11y
impact is contained in a single place.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Writing to and reading from monaco editors is a bit more
convoluted than making simple assertions on textareas.

The helpers should be re-usable for any place that uses
the monaco-json-editor.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Writing to and reading from monaco editors is a bit more
convoluted than making simple assertions on textareas.

The helpers should be re-usable for any place that uses
the monaco-json-editor.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
It takes any json, as primitives or arrays are also valid JSON logic.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Upgraded the builder to a version that uses the same
monaco-json-editor version to prevent different library
versions being included in the build.

This adds the explicit dependency on the monaco editor,
instead of relying on it being provided through
formio-builder.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
These components now use the underlying monaco-json-editor to edit
and/or preview the JSON data being manipulated. This fixes the annoying
autoformatting-while-typing and adds syntax highlighting, plus support
for dark/light theme.

Co-authored-by: @Viicos
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Sizing is based on number of rows/cols desired and set through a custom
CSS property on the respective components.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
* Moved around the position of widgets a bit, and declutter
* Instead of a toggle link, there's now a toggle icon
* Instead of expand/collapse, replace the editor UI with the Json widget
  and vice versa on toggle
* Ensure that icons have accessible labels when a title is provided
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Added a helper to put (JSON) expressions in the editors so that the
user interaction can be simulated. For speed reasons, it uses copy and
paste. Solution is taken from microsoft/playwright#14126
sergei-maertens added a commit that referenced this issue Jul 12, 2024
It leads to duplicate labels where the icon is used as a tooltip,
instead we can provide the prop/attribute in places where it's actually
waranted.

This refactors the toggle icon so that the message, behaviour and a11y
impact is contained in a single place.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Writing to and reading from monaco editors is a bit more
convoluted than making simple assertions on textareas.

The helpers should be re-usable for any place that uses
the monaco-json-editor.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
I could not for the life of me get this to work, after spending 4 hours
on it trying various permutations of workarounds:

* using a textarea or div that is contenteditable to grab the code from,
  via page.evaluate. Those actually all worked, but the problem wasn't
  getting the code/JSON on the clipboard - navigator.clipboard
  actually works just fine
* trying various combinations to get the code in the editor to clear:
  CTRL+A, CTRL+End (to end of document) followed by CTRL+Shift+Home to
  select everything from end to beginning of document, followed by
  Delete key to delete it -> can't get this selection to apply,
  with or without delays provided in keyboard.press

I've added numerous screenshots and compared them to what Chromium
browsers do, and that is different - the select-all-and-then-delete
to get rid of the existing content just doesn't seem to work. I don't
know if the editor is not properly initialized or events are not
properly dispatched, but I've given up at this point.

So, warning other users via an exception that this won't work on webkit
to save them the same debugging hassle and adding a skip helper for
these tests should still give us the coverage on chromium and firefox,
and manual testing on webkit will have to make do.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
Writing to and reading from monaco editors is a bit more
convoluted than making simple assertions on textareas.

The helpers should be re-usable for any place that uses
the monaco-json-editor.
sergei-maertens added a commit that referenced this issue Jul 12, 2024
I could not for the life of me get this to work, after spending 4 hours
on it trying various permutations of workarounds:

* using a textarea or div that is contenteditable to grab the code from,
  via page.evaluate. Those actually all worked, but the problem wasn't
  getting the code/JSON on the clipboard - navigator.clipboard
  actually works just fine
* trying various combinations to get the code in the editor to clear:
  CTRL+A, CTRL+End (to end of document) followed by CTRL+Shift+Home to
  select everything from end to beginning of document, followed by
  Delete key to delete it -> can't get this selection to apply,
  with or without delays provided in keyboard.press

I've added numerous screenshots and compared them to what Chromium
browsers do, and that is different - the select-all-and-then-delete
to get rid of the existing content just doesn't seem to work. I don't
know if the editor is not properly initialized or events are not
properly dispatched, but I've given up at this point.

So, warning other users via an exception that this won't work on webkit
to save them the same debugging hassle and adding a skip helper for
these tests should still give us the coverage on chromium and firefox,
and manual testing on webkit will have to make do.
sergei-maertens added a commit that referenced this issue Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment