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

Multiline Editor #2286

Merged
merged 7 commits into from
Apr 5, 2022
Merged

Multiline Editor #2286

merged 7 commits into from
Apr 5, 2022

Conversation

mason-fish
Copy link
Contributor

@mason-fish mason-fish commented Mar 24, 2022

fixes #1521

At long last, this finally introduces a multi-line editor to the app! The query-flow searchbar is now adaptive based on how many lines the query's value is: if it is only one line, we style the editor to appear as a searchbar, and if it is more than one then it will expand up to 3 lines with scrolling. In both cases we use the CodeMirror editor under the hood as the input type. I imported several useful extensions, including a couple that we are not yet using in this PR but plan on using later on as we continue the implementation (syntax highlighting, code folding, keyboard shortcuts, search within editor).

Currently looks like this:
(single)
image

(multi)
image

image

image

TODO:

  • Fix unnecessary app re-rendering on each input change
  • Hide behind feature flag
  • add back in a run/execute button in both stylings of the editor
  • fix cursor bugs

@mason-fish mason-fish changed the title integrate codemirror editor Multiline Editor Mar 24, 2022
@jameskerr
Copy link
Member

Updating the redux store just for the value of the search input should be fast enough. If that is re-rendering a bunch of things, we need to make sure that other components don't depend on that piece of state. I don't think anything else needs to depend on it. If they need to depend on the "last run" query, they can select the last ran version of the query.

@jameskerr
Copy link
Member

We should chat about where to put this UI state. I think we need a new reducer. I am wondering the same thing for where to store the pins UI state.

@mason-fish mason-fish force-pushed the 1521-multi-line-editor branch from 4bd4f16 to 08bf43d Compare March 29, 2022 18:00
@@ -51,9 +52,17 @@ export const getQuery = (state: State): BrimQuery | null => {
Queries.getQueryById(queryId)(state) ||
RemoteQueries.getQueryById(queryId)(state)
if (!query) return null
return new BrimQuery(query)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the culprit of all of the re-rendering, since it always creates a new object

@mason-fish mason-fish force-pushed the 1521-multi-line-editor branch from 08bf43d to f8cf120 Compare March 29, 2022 18:41
@mason-fish mason-fish marked this pull request as ready for review March 29, 2022 18:43
@mason-fish mason-fish requested a review from jameskerr March 29, 2022 18:43
@jameskerr
Copy link
Member

Awesome work @mason! Before I review the code, here are some visual things I'm noticing that could be improved.

It looks like the line-height is off if there are scrollbars . I have mine set to always on because I use that wacom tablet alot. When they are on it looks like it breaks the design. Set overflow: hidden on the container when there is only one line. That should remove the scroll bars.

Screen Shot 2022-03-29 at 12 14 40 PM

Before we merge, let's get the styles right on the editor. We'll want font-family: var(--mono-font). Here are the styles for it in sketch: https://www.sketch.com/s/c5a874f4-53b3-4af2-82fe-1212c45acda6/a/7yMzE08

And finally, instead of just giving them three lines when they switch to editor view, let's give them 150px. Something like this:

Screen Shot 2022-03-29 at 12 21 46 PM

In another PR, let's add the ability to resize the editor as large as they want. In a very future PR, we should let them put the editor and the results side by side, but that's just a thought I had right now.

Copy link
Member

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

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

Code looks good!

})
setView(newView)
newView.focus()
}, [ref])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, [ref])
}, [])

This will just run on mount because that ref object never changes its identity while the component is mounted.

You could do [ref.current] which will change, but the effect will run when something else causes it to render and ref.current has changed. As I'm sure you know, changing ref.current won't cause a re-render.

@mason-fish mason-fish requested a review from jameskerr March 30, 2022 17:40
Copy link
Member

@jameskerr jameskerr left a comment

Choose a reason for hiding this comment

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

I really like where we landed with this. It's ready. If you don't mind, let's just wait for the release to go out before merging.

@mason-fish mason-fish merged commit aac7f4b into main Apr 5, 2022
@mason-fish mason-fish deleted the 1521-multi-line-editor branch April 5, 2022 17:57
@philrz
Copy link
Contributor

philrz commented Apr 5, 2022

In case anyone else stumbles onto this and wants to check it out while it's still hidden behind a feature flag, I discovered the way to enable it in Dev mode:

yarn start --feature-flags=query-flow  

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.

Implement a Multi Line Editor for Brim
3 participants