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

draft: JENKINS-71527 - Update CodeMirror to v6 #8399

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Aug 19, 2023

See JENKINS-71527.

Jenkins relies on https://github.com/stapler/stapler-adjunct-codemirror to utilize code mirror.
This library is almost 10 years out of date and ships an ancient version of codemirror 2 (6 is the newest while writing this). Upgrading to v6 allows us to stop our usages of the above library as well as make use of all the new features in CodeMirror since then. There's now autocomplete, better accessibility, colours now use Jenkins variables and a whole load of other changes in the last ten years.

image

Draft as there's a lot of questions around this/work to do.

  1. With it being a large upgrade, I'm unsure if the textarea parameter codemirror-config will work the same way as before. What's the best option to handle this?
  2. The Pipeline plugin uses Ace - is there a reason for deviating from Jenkins, is there potential to settle on one library?

Things to do

  • Ensure compatibility with existing usages
  • Add resizing
  • Ensure readonly mode works

Testing done

  • Not done yet

Proposed changelog entries

  • Upgrade CodeMirror from 2.38 to v6.

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@janfaracik janfaracik changed the title draft: Update CodeMirror to v6 draft: JENKINS-71527 - Update CodeMirror to v6 Aug 19, 2023
@timja
Copy link
Member

timja commented Aug 19, 2023

Interesting comparison:
https://blog.replit.com/code-editors

I don't see a reason we need two, I assume codemirror was missing features at the time.

war/src/main/js/app.js Outdated Show resolved Hide resolved
@timja
Copy link
Member

timja commented Aug 19, 2023

script console size should start larger

@timja timja added web-ui The PR includes WebUI changes which may need special expertise major-rfe For changelog: Major enhancement. Will be highlighted on the top labels Aug 19, 2023
@janfaracik
Copy link
Contributor Author

script console size should start larger

Just for script console or for all CodeMirror editors?

@timja
Copy link
Member

timja commented Aug 19, 2023

script console size should start larger

Just for script console or for all CodeMirror editors?

probably all

@timja
Copy link
Member

timja commented Aug 20, 2023

run button is touching the script console textarea where before there was spacing

image

I think it could do with a larger minHeight too
see before:

image

@timja
Copy link
Member

timja commented Aug 20, 2023

Not a blocker but I think the theme the dark-theme is using is easier to read on code mirror 4:

This PR:
image

Master with dark-theme:
image

From this gist: https://gist.githubusercontent.com/timja/04afb12c8ad909e400317a2ad9c88445/raw/7a9433120150425d3e944074227c215dcce81181/jenkins-dump-credentials.groovy

@timja timja added the ath-fail The acceptance-test-harness suite needs a forward-compatible change label Aug 20, 2023
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 23, 2023
@github-actions
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@daniel-beck
Copy link
Member

Given the title, is this supposed to be in the draft state?

@timja timja marked this pull request as draft August 23, 2023 14:58
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 2, 2023
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2023

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 17, 2023
textarea.style.display = "none";
if (textarea.form) {
textarea.form.addEventListener("submit", () => {
textarea.value = view.state.doc.toString();
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to be working, at least for shell steps on freestyle builds.

It gets called but the new value isn't submitted.
Also doesn't get called at all on Apply button clicks

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 29, 2023
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@janfaracik
Copy link
Contributor Author

Will come back to this.

@janfaracik janfaracik closed this Jan 4, 2025
@basil basil reopened this Jan 18, 2025
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-fail The acceptance-test-harness suite needs a forward-compatible change major-rfe For changelog: Major enhancement. Will be highlighted on the top web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants