Skip to content

Conversation

@logonoff
Copy link
Member

@logonoff logonoff commented Jan 7, 2025

CONSOLE Features and Fixes:

https://issues.redhat.com/browse/CONSOLE-4407
https://issues.redhat.com/browse/CONSOLE-4409

Solution description

Screen shots / gifs / design review:

Before:
image
image

After:
image
image

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 7, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 7, 2025

@logonoff: This pull request references CONSOLE-4407 which is a valid jira issue.

Details

In response to this:

CONSOLE Features and Fixes:

https://issues.redhat.com/browse/CONSOLE-4407

Solution description

This PR lays the foundation for switching to the PatternFly Code editor by doing the following:

Screen shots / gifs / design review:

Before:
image

After:
image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci openshift-ci bot requested review from Lucifergene and kyoto January 7, 2025 15:55
@openshift-ci openshift-ci bot added component/core Related to console core functionality component/dev-console Related to dev-console component/sdk Related to console-plugin-sdk component/shared Related to console-shared labels Jan 7, 2025
@logonoff logonoff force-pushed the CONSOLE-4407-monaco branch from 3b9e426 to 03796ad Compare January 7, 2025 15:56
@logonoff
Copy link
Member Author

logonoff commented Jan 7, 2025

debt, adding labels

/label px-approved
/label docs-approved

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Jan 7, 2025
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

@logonoff Nice work!

Do we know if this impacts any public plugin APIs? It looks like we do expose Monaco options in the CodeEditor SDK component:

https://github.com/openshift/console/blob/master/frontend/packages/console-dynamic-plugin-sdk/docs/api.md#codeeditor

cc @vojtechszocs

Copy link
Member

Choose a reason for hiding this comment

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

Love seeing all of this code removed

@spadgett
Copy link
Member

spadgett commented Jan 7, 2025

And it looks like we expose the editor ref itself, so if there are breaking changes to the Monaco API, it can affect plugins. I think we have to bite the bullet if there are since not ever upgrading Monaco is not an option. We'll need to minimally release note this change, however.

@logonoff logonoff force-pushed the CONSOLE-4407-monaco branch 6 times, most recently from 6c6d8c8 to 92b628b Compare January 8, 2025 21:43
@openshift-ci openshift-ci bot added the component/pipelines Related to pipelines-plugin label Jan 8, 2025
@logonoff logonoff force-pushed the CONSOLE-4407-monaco branch from 92b628b to e832153 Compare January 8, 2025 21:46
@openshift-ci openshift-ci bot added the component/knative Related to knative-plugin label Jan 8, 2025
@logonoff
Copy link
Member Author

QE review:

/assign @yapei

@logonoff logonoff force-pushed the CONSOLE-4407-monaco branch 2 times, most recently from a50c157 to 8e2d1e0 Compare January 17, 2025 15:00
@logonoff logonoff force-pushed the CONSOLE-4407-monaco branch from 8e2d1e0 to b005a91 Compare January 23, 2025 14:48
@logonoff logonoff force-pushed the CONSOLE-4407-monaco branch 2 times, most recently from 2b7a857 to 437c76f Compare February 7, 2025 15:01
@sanketpathak
Copy link
Contributor

@logonoff Are these two expected behaviors:

  1. With the command palette option I used the cursor above option which created two cursors and is confusing to the end user, this is also happening with other options in the palette
Screencast.From.2025-02-19.19-26-55.mp4
  1. With the reduced size of yaml editor it restricts editing as the window does not remain stable at the cursor position
Screencast.From.2025-02-19.19-30-37.mp4

@logonoff
Copy link
Member Author

@logonoff Are these two expected behaviors:

1. With the command palette option I used the cursor above option which created two cursors and is confusing to the end user, this is also happening with other options in the palette

Yes, the Add cursor above option is supposed to create another cursor. This feature also exists in the current monaco editor

2. With the reduced size of yaml editor it restricts editing as the window does not remain stable at the cursor position

Screencast.From.2025-02-19.19-30-37.mp4

Yes, this behavior is consistent with the current monaco editor. I'll add a minimum height for the CodeEditor so that the cursor is always visible

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 19, 2025

@logonoff: This pull request references CONSOLE-4407 which is a valid jira issue.

This pull request references CONSOLE-4409 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

Details

In response to this:

CONSOLE Features and Fixes:

https://issues.redhat.com/browse/CONSOLE-4407
https://issues.redhat.com/browse/CONSOLE-4409

Solution description

Screen shots / gifs / design review:

Before:
image
image

After:
image
image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@yanpzhan
Copy link
Contributor

yanpzhan commented Feb 20, 2025

@logonoff check on cluster launched against the pr, tooltips will not show up for 'kind' and 'apiversion' on existing yaml file editor, when create new yaml, hover on 'kind' and 'apiversion', tooltips will show up. If it's an issue? Seems it occurs when yaml is long. Didn't reproduce with Secret and Configmap yaml.
tooltips not shown:
Screenshot from 2025-02-20 15-40-19
tooltips shown:
Screenshot from 2025-02-20 15-39-55

@logonoff
Copy link
Member Author

logonoff commented Feb 20, 2025

@logonoff check on cluster launched against the pr, tooltips will not show up for 'kind' and 'apiversion' on existing yaml file editor, when create new yaml, hover on 'kind' and 'apiversion', tooltips will show up. If it's an issue? Seems it occurs when yaml is long. Didn't reproduce with Secret and Configmap yaml. tooltips not shown: Screenshot from 2025-02-20 15-40-19 tooltips shown: Screenshot from 2025-02-20 15-39-55

Hey @yanpzhan, I discussed with @jhadvig and it should be fine to not have the tooltips since the information is there on the autocomplete tooltip:
image

There is a new feature, sticky scroll, that is introduced with this PR:
image

Do you think that an option needs to exist to toggle it? I'm asking since we have a tooltip toggle as well, and I read in the ticket adding the toggle that it was because it takes up too much space in the UI and interferes with editing, which this feature might possibly interfere with as well.

@jhadvig and I are leaning towards not adding a toggle, but I want to know your thoughts too

@yanpzhan
Copy link
Contributor

@logonoff Thanks for your answer. The tooltip toggle helps when user edits the yaml, I'm happy if there is toggle for enabling/disabling 'sticky scroll', because I somehow don't care to know starting lines of currently visible nested contexts, and I feel the height of effective current lines are reduced.

@logonoff
Copy link
Member Author

@logonoff Thanks for your answer. The tooltip toggle helps when user edits the yaml, I'm happy if there is toggle for enabling/disabling 'sticky scroll', because I somehow don't care to know starting lines of currently visible nested contexts, and I feel the height of effective current lines are reduced.

Thanks for your thoughts! Do you think it's okay to implement this in a follow up PR to avoid further increasing the scope of this large PR, and so I can also gather some thoughts from UX as well?

Also, other than the issues mentioned, are there any other problems you've noticed with the updated CodeEditor?

@yanpzhan
Copy link
Contributor

yanpzhan commented Feb 25, 2025

Do you think it's okay to implement this in a follow up PR to avoid further increasing the scope of this large PR, and so I can also gather some thoughts from UX as well?

It's ok for me.
There is not other issue from my side.
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 25, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 25, 2025

@logonoff: This pull request references CONSOLE-4407 which is a valid jira issue.

This pull request references CONSOLE-4409 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

Details

In response to this:

CONSOLE Features and Fixes:

https://issues.redhat.com/browse/CONSOLE-4407
https://issues.redhat.com/browse/CONSOLE-4409

Solution description

Screen shots / gifs / design review:

Before:
image
image

After:
image
image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

- Remove `Alt+F1` a11y help shortcut hint since the shortcut no longer exists in monaco
- Fix long time bug where the commands in the command palette are bolded and underlined
@logonoff logonoff force-pushed the CONSOLE-4407-monaco branch from 5734187 to c2d33df Compare March 3, 2025 20:21
Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logonoff, TheRealJon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 4, 2025

@logonoff: This pull request references CONSOLE-4407 which is a valid jira issue.

This pull request references CONSOLE-4409 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

Details

In response to this:

CONSOLE Features and Fixes:

https://issues.redhat.com/browse/CONSOLE-4407
https://issues.redhat.com/browse/CONSOLE-4409

Solution description

Screen shots / gifs / design review:

Before:
image
image

After:
image
image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 4, 2025

@logonoff: This pull request references CONSOLE-4407 which is a valid jira issue.

This pull request references CONSOLE-4409 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

Details

In response to this:

CONSOLE Features and Fixes:

https://issues.redhat.com/browse/CONSOLE-4407
https://issues.redhat.com/browse/CONSOLE-4409

Solution description

Screen shots / gifs / design review:

Before:
image
image

After:
image
image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 4, 2025

@logonoff: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 6b8cba2 into openshift:main Mar 5, 2025
8 checks passed
@logonoff logonoff deleted the CONSOLE-4407-monaco branch March 5, 2025 00:51
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-console
This PR has been included in build openshift-enterprise-console-container-v4.19.0-202503050640.p0.g6b8cba2.assembly.stream.el9.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/pipelines Related to pipelines-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/cypress Related to Cypress e2e integration testing kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.