-
Notifications
You must be signed in to change notification settings - Fork 77
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
Ember v3.28.0...v5.2.0 #402
Conversation
639dcbe
to
8bb133d
Compare
FYI to reviewers, this would start the breaking changes on the road to a v5: #400 |
@@ -62,8 +62,6 @@ module.exports = { | |||
disableDecoratorTransforms: false, | |||
enableTypeScriptTransform: true, | |||
|
|||
throwUnlessParallelizable: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we didn't leave this? if we aren't parallelizable something bad has gone wrong. I've worked pretty hard to keep things parallelizable around the ecosystem opening fixes when hitting issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to trace the error back to the root cause. I can give it another look, but I'm going to consider this non-blocking and decouple the investigation. Frankly I considered this option as being old enough that I wasn't sure it was still something meaningful.
A conversion of this whole project to a v2 addon would simplify the dependencies and I presume make the build parallelization concerns moot (at least for consumers of this library), and I suspect that is the meta.
@@ -1,8 +1,8 @@ | |||
<header class="container-fluid"> | |||
<div class="row"> | |||
<h1 class="col-sm-6"> | |||
<LinkTo @route="index"> | |||
<img alt="HTML Next Logo" src="./HTML-Next.png" style="height: 2em;"> | |||
<LinkTo @route="index" style="color:#222"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we end up needing the extra color?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link is an a
tag and turns blue, but this is the logo on the top left in the header. We want to to be the same color as the logo image, which is #222
.
This was probably broken in 3343ecc#diff-a6b32449eac6431ef7263de086a744f8aed3883a686fe93880e607f5e08ffdb9L3, although I don't believe the docs site after that change was ever deployed (it doesn't seem like it, at least).
I agree this is a bit lame, but I think you agree we have bigger fish to fry.
}, | ||
|
||
loadBelow() { | ||
let last = this.model.last; | ||
let last = this.model.data.last; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry for the sins that I committed
@@ -23,8 +22,6 @@ module('vertical-collection', 'Integration | Modern Ember Features Tests', funct | |||
</div> | |||
`); | |||
|
|||
await settled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I dislike this lint rule as its actually dishonest. But also yay cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no blocking feedback, left a few nits inline.
d037092
to
c659a6c
Compare
c659a6c
to
00e9f33
Compare
Also