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

feat: word-wrap editor #355

Merged
merged 15 commits into from
Nov 21, 2024
Merged

feat: word-wrap editor #355

merged 15 commits into from
Nov 21, 2024

Conversation

cristianoventura
Copy link
Collaborator

Description

This PR enables word-wrap for the Monaco editor in Script Preview and the Content tab of a response. The state for both are persisted individually in the settings file.

How to Test

  • open Script Preview and toggle the word-wrap switch
  • open the response content of a JSON request and toggle word-wrap
  • re-open the app and check that the previous state was persisted

Checklist

  • I have performed a self-review of my code.
  • [] I have added tests for my changes.
  • I have run linter locally (npm run lint) and all checks pass.
  • I have run tests locally (npm test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

Screen.Recording.2024-11-18.at.3.14.10.PM.mov

Related PR(s)/Issue(s)

Resolves #237

@cristianoventura cristianoventura self-assigned this Nov 18, 2024
@cristianoventura cristianoventura requested a review from a team as a code owner November 18, 2024 20:52
@cristianoventura cristianoventura requested review from going-confetti, e-fisher and Llandy3d and removed request for a team November 18, 2024 20:52
import { useState, useEffect } from 'react'
import { useDebounce } from 'react-use'

export function useEditorWordWrapSetting(key: keyof EditorSettings) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This hook can be used to handle each setting independently based on the key (wordWrapScriptPreview or wordWrapResponseContent)

Comment on lines 10 to 15
async function fetchSettings() {
const settings = await window.studio.settings.getSettings()
setEditor(settings.editor)
setWordWrap(settings.editor[key])
}
fetchSettings()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should use react query to fetch settings, so that they are cached and only re-requested when the cache is invalidated (after changing a setting, for example). This is probably out of scope of this PR, but would simplify working with settings elsewhere. Though I'm curious what others think.

Comment on lines 103 to 104
wordWrapScriptPreview: WordWrapSchema,
wordWrapResponseContent: WordWrapSchema,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about exposing these in the Settings dialog?

<Flex p="2" gap="2" justify="end">
<Flex p="2" gap="2" justify="between">
<Label flexGrow="1">
<Text size="2">Word-wrap</Text>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Text size="2">Word-wrap</Text>
<Text size="2">Word wrap</Text>

Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

I feel like word wrap toggle should be a part of CodeEditor component, perhaps a Toolbar component inside, with option to control it through props.

Also, I probably wouldn't go as far as having this option saved per every component, just drop it in local storage to remember my preferred view and apply in all places we show editor. It's only one click away to toggle, anyway. IMO, added complexity doesn't outweigh benefit added, but would love to hear what others think.

Copy link
Collaborator Author

@cristianoventura cristianoventura left a comment

Choose a reason for hiding this comment

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

I've followed @e-fisher's suggestion to keep the state in local storage. This indeed minimizes complexity. I also implemented a EditorToolbar component to hold the word wrap switch so now all instances of Monaco editor can have it.

image

cc @going-confetti

Llandy3d
Llandy3d previously approved these changes Nov 20, 2024
Copy link
Member

@Llandy3d Llandy3d left a comment

Choose a reason for hiding this comment

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

Looks good! 🙌

I wonder if it makes sense to have it on a smaller size on the right side of the bar UI wise because I assume it is a "set and forget" element 🤔

Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

Agree with Llandy on moving it to the right. Maybe not just make it smaller, but use an icon with a tooltip instead?

Found a weird bug when toggle doesn't when viewing raw request response, here's a video:

CleanShot.2024-11-20.at.12.09.51.mp4

Otherwise looks great!

@cristianoventura
Copy link
Collaborator Author

cristianoventura commented Nov 20, 2024

I've moved the word wrap switch to an IconButton aligned at the end of the toolbar.

image

@e-fisher I noticed that this only happens for JSON >= than 10000 characters. It could be related to internal performance and how Monaco handles it. I'm still trying to figure out how to solve this issue.

const [toolbarState, setToolbarState] = useState<ToolbarState>({
wordWrap: 'off',
})

// Monaco automatically applies word wrap if the content length of a line is >= 10000 characters
// In this case, we disable the word wrap button so the internal state is respected
const shouldEnableWordWrapButton = () => {
Copy link
Collaborator Author

@cristianoventura cristianoventura Nov 20, 2024

Choose a reason for hiding this comment

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

@e-fisher This was an interesting find. If any line has a content length >= 10000 characters, Monaco overrides the wordWrap option. This is likely related to stopRenderingLineAfter but changing this option doesn't seem to have any effect on this case specifically other than hiding the rest of the line content.

This was the solution I came up with by disabling the word wrap button and respecting Monaco's internal state. You can test with HTML content as well such as the home page of grafana.com.

Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting!
I think respecting that makes sense 🤔

Llandy3d
Llandy3d previously approved these changes Nov 20, 2024
e-fisher
e-fisher previously approved these changes Nov 21, 2024
Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

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

Great find, and sorry for taking you down the rabbit hole! 😅

I would wrap the line calculation in useMemo to avoid running on every re-render, but otherwise looks good!

@cristianoventura cristianoventura merged commit 61a25ff into main Nov 21, 2024
2 checks passed
@cristianoventura cristianoventura deleted the feat/word-wrap-monaco-editor branch November 21, 2024 16:44
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.

Toggle word-wrap in Monaco editor
4 participants