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 monaco-editor for code display #173

Merged
merged 12 commits into from
Nov 13, 2023
Merged

Use monaco-editor for code display #173

merged 12 commits into from
Nov 13, 2023

Conversation

carbonrobot
Copy link
Contributor

@carbonrobot carbonrobot commented Nov 9, 2023

Use monaco-editor for code display instead of @microlink/react-json-view.

We want to remove the current editor to resolve the dependency issues with npm. See #171 for more information. Monaco Editor is the tool that powers the VS Code Editor. Having a lightweight editor allows us the opportunity to allow editing and replaying in the UX in a future update and provides built in language support/syntax highlighting for multiple response types.

  • Provides a <CodeDisplay> component that uses the content-type header value to determine the language for syntax highlighting
  • Fixes an issue where large auth tokens that are large expand vertically
  • Sets a fixed height for the decoded token view. The editor is lazy loaded and can't determine its height with flex. This also prevents it from expanding infinitely with large token data sets

Before

image

After

image

Also fixes an issue where large auth tokens that are large expand vertically

Before

image

After

image

Copy link

changeset-bot bot commented Nov 9, 2023

🦋 Changeset detected

Latest commit: c3b16d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@envyjs/webui Minor
@envyjs/apollo Minor
@envyjs/core Minor
@envyjs/nextjs Minor
@envyjs/node Minor
@envyjs/web Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

kgpax
kgpax previously requested changes Nov 9, 2023
Copy link
Member

@kgpax kgpax left a comment

Choose a reason for hiding this comment

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

I'm in favour of the change to use this for the code views. Some observations though:

  • It looks like there is an error thrown when trying to view this mock trace... perhaps something with the JSON which throws an error when using this component?

image

image

  • There seems to be an unnecessary overscroll. For example, a mock which only has a 4 line response can be scrolled down, despite the full response being comfortably visible in the container.

CleanShot 2023-11-09 at 20 57 08

  • The previous react-json-view had some nice little UX features which it would be nice to retain, if at all possible with this replacement...

  • Collapse the JSON at a certain level by default. This means that responses with deeply nested objects doesn't need to be fully expanded, and expansion can be a little more selective, letting the later response properties be visible:

image

  • Automatically truncate large values, and expand on click. Wasn't sure I was sold on this when I first used it, but it did make scanning the object easier, and often times we only need to see the initial part of each value to know if we want to see the full value.

image

One final bug: switching tabs tries to persist that selected tab when viewing other traces. This, however, is a problem if the trace we switch to does not have that tab. For example; with the mock data...

  • View a valid trace
  • Change to the "Response" tab
  • Change the trace to one of the error ones (which doesn't have a response)
  • The response from the previous trace remains visible
  • You have to explicitly switch to the "Details" tab to see the correct trace information

CleanShot 2023-11-09 at 21 00 56

@carbonrobot
Copy link
Contributor Author

@kgpax good call, will see what we can do here

@carbonrobot
Copy link
Contributor Author

@kgpax Updated. I was unable to find a solution to "Automatically truncate large values". The only options are word wrap, but thats not really the same thing. Play around with the current build and let me know what you think.

@@ -105,7 +105,7 @@ export default function Authorization({ value }: AuthorizationProps) {
}
})()}
{tokenState !== TokenState.Minimal && (
<div className={tw('flex flex-row gap-2 bg-slate-100 px-4 pt-4')}>
<div className={tw('flex flex-row gap-2 bg-gray-200 px-4 pt-4')}>
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if this had a hover state which re-enforced that the data is interactive. I.e., in the original design, this would have a hove state like this:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -20,8 +20,14 @@ import ResponseHeaders from './ResponseHeaders';
import { TabContent, TabList, TabListItem } from './Tabs';
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's related to these code changes, but it does look to be related to a new mock trace that had been added at some point:

image

This is the mock GQL "PDP" data trace:

import { Event, HttpRequestState } from '@envyjs/core';

And if you look at the screenshot, it doesn't look like its parsing and presenting the query correctly. Compare this with the mock trace the GQL "People" query:

image

Also, in this above example, the code display part should really be full width, even if the content is narrow. It looks... odd... to make the box narrow like this IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will address in #176

@@ -20,8 +20,14 @@ import ResponseHeaders from './ResponseHeaders';
import { TabContent, TabList, TabListItem } from './Tabs';
import TimingsDiagram from './TimingsDiagram';
Copy link
Member

Choose a reason for hiding this comment

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

Also not quite related to these code changes, but for the "Aborted" and "Timeout" examples, maybe we shouldn't include empty the "Response Headers" and "Timing" sections:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -105,7 +105,7 @@ export default function Authorization({ value }: AuthorizationProps) {
}
})()}
{tokenState !== TokenState.Minimal && (
<div className={tw('flex flex-row gap-2 bg-slate-100 px-4 pt-4')}>
<div className={tw('flex flex-row gap-2 bg-gray-200 px-4 pt-4')}>
Copy link
Member

Choose a reason for hiding this comment

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

On expansion, the display mode and the collapse buttons don't look great. Not sure whether there is a plan to address this in another PR?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryansrofe has updated designs for this. Will address in #175

});
monaco.editor.setTheme('envy');

executeFolding();
Copy link
Member

Choose a reason for hiding this comment

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

When the editor is in view, we get a flash of the folding happening. Not the end of the world, but is there anything that can be done to avoid it?

CleanShot 2023-11-13 at 09 20 06

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some time on this, but there is no simple solutions. Will continue investigation as part of #174

@kgpax
Copy link
Member

kgpax commented Nov 13, 2023

@kgpax Updated. I was unable to find a solution to "Automatically truncate large values". The only options are word wrap, but thats not really the same thing. Play around with the current build and let me know what you think.

I think the truncation in the previous component was there because the default behaviour was to wrap the text (i.e., a long value would wrap over multiple lines)... therefore the truncation made it better because each line was a different property.

Given that this editor has word-wrap disabled, we kind of get that same overall experience, so on reflection I'd say that the truncation isn't really necessary any more.

@kgpax kgpax dismissed their stale review November 13, 2023 09:26

Removing blocking review; critical bug has been fixed

@carbonrobot
Copy link
Contributor Author

Preview Package Publishing

🦋  success packages published successfully:
🦋  @envyjs/[email protected]
🦋  @envyjs/[email protected]
🦋  @envyjs/[email protected]
🦋  @envyjs/[email protected]
🦋  @envyjs/[email protected]
🦋  @envyjs/[email protected]

@carbonrobot carbonrobot merged commit 2bc08f4 into main Nov 13, 2023
1 check passed
@carbonrobot carbonrobot deleted the ui/code-display branch November 13, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants