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

UI: convert to angle bracket invocation #8075

Merged
merged 9 commits into from
Jun 1, 2020
Merged

Conversation

backspace
Copy link
Contributor

@backspace backspace commented May 29, 2020

This is mostly a direct application of the ember-angle-brackets-codemod.
I manually restored newlines in multi-line component invocations, mostly
preserving file line length except as noted.

I needed to rename task to taskState in some cases to avoid Ember
Concurrency naming conflicts like this:
https://app.circleci.com/pipelines/github/hashicorp/nomad/9595/workflows/69183c14-c50d-4076-a1f7-da5a36fb8b24/jobs/71633/tests

This is the result of running the codemod and manually inserting
line breaks where there were previously multiline invocations.
I used file line length as a guide, aiming to keep them the same,
with one exception: app/templates/components/gutter-menu.hbs,
where I made the LinkTo @route instances be on their own lines
which added two lines to the file.

There was an error processing one file, as in
ember-codemods/ember-angle-brackets-codemod#185

Uses of link-to with standalone data attributes were also missed
ember-codemods/ember-angle-brackets-codemod#237
@github-actions
Copy link

github-actions bot commented May 29, 2020

Ember Asset Size action

As of 8fe0dff

Files that got Smaller 🎉:

File raw gzip
nomad-ui.js -34.9 kB -1.51 kB

Files that stayed the same size 🤷‍:

File raw gzip
auto-import-fastboot.js 0 B 0 B
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented May 29, 2020

Ember Test Audit comparison

master 8fe0dff change
passes 1353 1354 +1
failures 0 0 0
flaky 1 0 -1
duration 5m 00s 340ms 5m 07s 349ms +07s 009ms

This is the result of running the codemod with this config:
{
  "includeValuelessDataTestAttributes": true
}

I applied the same line-length-preserving manual formatting
as in the previous commit. Thanks to @steventsao for
ember-codemods/ember-angle-brackets-codemod#241
This fixes errors such as these:
https://app.circleci.com/pipelines/github/hashicorp/nomad/9595/workflows/69183c14-c50d-4076-a1f7-da5a36fb8b24/jobs/71633/tests

It seems that there’s now a shadowing situation where {{task}}
is being interpreted as an Ember Concurrency helper…? Since
this is actually a task state anyway, easier to just rename it.
This is a manual fix for this instance missed because of
ember-codemods/ember-angle-brackets-codemod#185
# Conflicts:
#	ui/app/routes/allocations/allocation/task/fs.js
#	ui/app/templates/allocations/allocation/task/fs.hbs
#	ui/app/templates/components/fs-breadcrumbs.hbs
#	ui/app/templates/components/fs/directory-entry.hbs
I followed the same pattern as in be60dff. I’ll need to
apply the same types of changes as in 2078c2a now.
This is a repetition of 2078c2a post-merge.
Again as in be60dff, this includes manual updates to preserve
multi-line invocation. I also updated task-log-test to rename
task to taskState as in 2078c2a.
Jobs
{{/link-to}}
</LinkTo>
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 is an example of the pattern where the invocation becomes one line longer because @route is not a positional argument like it was with curly bracket invocation and it seemed to me to be more consistent to have it on a separate line as with other named arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this. Consistency over line count every time.

@backspace backspace marked this pull request as ready for review June 1, 2020 16:56
Copy link
Contributor

@DingoEatingFuzz DingoEatingFuzz left a comment

Choose a reason for hiding this comment

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

It's beautiful 😍

@backspace backspace merged commit 67e9690 into master Jun 1, 2020
@backspace backspace deleted the f-ui/angle-brackets branch June 1, 2020 19:03
backspace added a commit that referenced this pull request Jul 9, 2020
This includes fixes for newer template lint rules that came along with
updating that dependency, which was necessary to be able to use
the no-curly-component-invocation rule. It also updates some curly
invocations that I missed in #8075.
@github-actions
Copy link

github-actions bot commented Jan 4, 2023

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 Jan 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants