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

Replaced other instances of IvyCodeMirror throughout the app #13528

Merged
merged 7 commits into from
Jun 30, 2022

Conversation

philrenaud
Copy link
Contributor

@philrenaud philrenaud commented Jun 29, 2022

Twig of #13441 — replaces all old instances of <IvyCodemirror> with the new modifier, and makes some small edit to the codemirror modifier itself.

@github-actions
Copy link

github-actions bot commented Jun 29, 2022

Ember Asset Size action

As of 858b08f

Files that got Smaller 🎉:

File raw gzip
nomad-ui.js -1.68 kB -264 B
vendor.js -200 kB -64.8 kB
vendor.css -378 B -70 B

Files that stayed the same size 🤷‍:

File raw gzip
nomad-ui.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Jun 29, 2022

Ember Test Audit comparison

13441-edit-as-json 858b08f change
passes 1326 1326 0
failures 2 2 0
flaky 0 0 0
duration 000ms 000ms -000ms

Copy link
Contributor

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

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

There's some more deprecations and cleaning up we can do while we're here.

mode="javascript"
{{code-mirror
screenReaderLabel="Job definition"
content=(or this.job._newDefinition this.jobSpec)
Copy link
Contributor

Choose a reason for hiding this comment

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

blocking question: JobEditor doesn't have a jobSpec property and neither of these properties are reactive, however, we call set on _newDefintion which might do the trick.

Do we know where this.jobSpec is coming from? Searching the codebase, this is the only use of jobSpec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it used to be something that could be passed in and just never got cleaned up (it's present in the IvyCodemirror code, too)

I'll remove it here

Copy link
Contributor

Choose a reason for hiding this comment

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

This could introduce a potential breaking change and thankfully it'll be shipped in a major release. But, I'd love to chat with @lgfa29 re: UI release process.

@lgfa29 - we have some serious re-designs for the Job Detail Overview page (this PR is unrelated to that) coming down the pipe. But, this PR is a good example of potential breaking API changes in our UI code. Should we start coordinating with the release process for these types of things?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need any special coordination. Since this is all set to go out with 1.4.0 all we need is for the changes to be in main before the release.

One thing we can do is to update bindata_assetfs.go in main so that anyone running make dev will see the latest UI changes before the release. This can help get more eyes and test major changes before the beta.

ui/app/templates/components/json-viewer.hbs Outdated Show resolved Hide resolved
return cm;
export function getCodeMirrorInstance() {
return function () {
return document.querySelector('.CodeMirror').CodeMirror;
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: We can deprecate this helper entirely now this its just selecting a DOM element without tapping into the container registry or throwing an error.

Its used in 3 files. And we should be able to write:

const expected = document.querySelector('.CodeMirror').CodeMirror.value;
const actual = JSON.stringify(result, null, 2);

assert.equals(expected, actual);

ui/app/components/secure-variable-form.js Outdated Show resolved Hide resolved
@philrenaud philrenaud merged commit 4158224 into 13441-edit-as-json Jun 30, 2022
@philrenaud philrenaud deleted the 13441-replace-ivy-codemirror branch June 30, 2022 15:18
philrenaud added a commit that referenced this pull request Jul 4, 2022
* Toying with insert and update helpers before translation func

* Working prototype that lets you switch between json and tabular

* No longer add the bonus items row in json mode

* Trimmed the ivy from the codemirror (#13503)

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger

* Replaced other instances of IvyCodeMirror throughout the app (#13528)

* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup

* JSON editor added to /new and all variables stringified before save or translate

* Give users a foothold when editing an empty item in JSON mode

* Copy the empty KV

* No duplicate keys in KV

* Better handling of cursor snapping in json edit field

* Catch formatting errors on the fly

* Basic tests for JSON to Table and Table to JSON in form
tgross pushed a commit that referenced this pull request Jul 8, 2022
* Toying with insert and update helpers before translation func

* Working prototype that lets you switch between json and tabular

* No longer add the bonus items row in json mode

* Trimmed the ivy from the codemirror (#13503)

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger

* Replaced other instances of IvyCodeMirror throughout the app (#13528)

* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup

* JSON editor added to /new and all variables stringified before save or translate

* Give users a foothold when editing an empty item in JSON mode

* Copy the empty KV

* No duplicate keys in KV

* Better handling of cursor snapping in json edit field

* Catch formatting errors on the fly

* Basic tests for JSON to Table and Table to JSON in form
tgross pushed a commit that referenced this pull request Jul 8, 2022
* Toying with insert and update helpers before translation func

* Working prototype that lets you switch between json and tabular

* No longer add the bonus items row in json mode

* Trimmed the ivy from the codemirror (#13503)

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger

* Replaced other instances of IvyCodeMirror throughout the app (#13528)

* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup

* JSON editor added to /new and all variables stringified before save or translate

* Give users a foothold when editing an empty item in JSON mode

* Copy the empty KV

* No duplicate keys in KV

* Better handling of cursor snapping in json edit field

* Catch formatting errors on the fly

* Basic tests for JSON to Table and Table to JSON in form
tgross pushed a commit that referenced this pull request Jul 11, 2022
* Toying with insert and update helpers before translation func

* Working prototype that lets you switch between json and tabular

* No longer add the bonus items row in json mode

* Trimmed the ivy from the codemirror (#13503)

* Trimmed the ivy from the codemirror

* editedJSONItems removal

* De-debugger

* Replaced other instances of IvyCodeMirror throughout the app (#13528)

* Replaced other instances of IvyCodeMirror throughout the app

* PR requests for codemirror modifier

* Screen reader setting as param

* Trying a simpler codemirror test helper

* Lint removal

* Screen Reader Label added for a11y

* JSONViewer cleanup

* JSON editor added to /new and all variables stringified before save or translate

* Give users a foothold when editing an empty item in JSON mode

* Copy the empty KV

* No duplicate keys in KV

* Better handling of cursor snapping in json edit field

* Catch formatting errors on the fly

* Basic tests for JSON to Table and Table to JSON in form
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants