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 in ConfigMap details #6830

Merged
merged 12 commits into from
Dec 29, 2022

Conversation

aleksfront
Copy link
Contributor

Fixes #6815 providing more useful editor with Tab key support for ConfigMap data entries.

config.map.monaco.editor.mov

Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
@aleksfront aleksfront added enhancement New feature or request area/ui labels Dec 26, 2022
@aleksfront aleksfront added this to the 6.4.0 milestone Dec 26, 2022
@aleksfront aleksfront requested a review from a team as a code owner December 26, 2022 10:09
ixrock
ixrock previously approved these changes Dec 27, 2022
Copy link
Contributor

@ixrock ixrock left a comment

Choose a reason for hiding this comment

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

Some questions.. Otherwise LGTM.

Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
ixrock
ixrock previously approved these changes Dec 28, 2022
Copy link
Contributor

@ixrock ixrock left a comment

Choose a reason for hiding this comment

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

LGTM


export type MonacoEditorSize = (linesCount: number) => number;

const getEditorHeightFromLinesCountInjectable = getInjectable({
Copy link
Contributor

@ixrock ixrock Dec 28, 2022

Choose a reason for hiding this comment

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

So why in general do we need this function/feature to be *.injectable.ts?
Any useful examples of how it would be utilized later in other envs? Or what's purpose in other words?

Copy link
Contributor Author

@aleksfront aleksfront Dec 29, 2022

Choose a reason for hiding this comment

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

It is implemented as injectable to follow de-facto standard and being able to inject it as dependency into MonacoEditor:

export const MonacoEditor = withInjectables<Dependencies, MonacoEditorProps, MonacoEditorRef>(
  React.forwardRef<MonacoEditorRef, MonacoEditorProps & Dependencies>((props, ref) => <NonInjectedMonacoEditor innerRef={ref} {...props} />),
  {
    getProps: (di, props) => ({
      ...props,
      userStore: di.inject(userStoreInjectable),
      activeTheme: di.inject(activeThemeInjectable),
      getEditorHeightFromLinesCount: di.inject(getEditorHeightFromLinesCountInjectable),
    }),
  },
);

Without injectable format it would be used from the shared state (eg. imported module) which is considered as bad practice.

Copy link
Contributor

@ixrock ixrock Dec 29, 2022

Choose a reason for hiding this comment

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

Alright, but why you've decided to control only height from MonacoEditor and this is why it's moved to injectable.ts?
You still didn't answer my question: if you able to control height of the editor, when/where would be reused?
Some examples? Maybe builds for lens-ide or lens-some-build with different initial heights for the editor? This would make sense?

Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
@aleksfront aleksfront merged commit 6422d81 into master Dec 29, 2022
@aleksfront aleksfront deleted the use-monaco-in-configmap-details branch December 29, 2022 11:34
@Nokel81 Nokel81 mentioned this pull request Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick-view panel editors should have a fixed font, and a better editor
2 participants