kredits/kredits-web
9
0
Fork 0
Code Issues 22 Pull Requests 1 Actions Releases 18 Activity

Upgrade Ember and deps to 3.18/Octane #186

Merged
raucao merged 15 commits from chore/upgrade_ember_and_deps into master 2020-05-11 11:18:59 +00:00
Conversation 10 Commits 15 Files Changed 68
+8979 -3357
raucao commented 2020-05-11 08:39:29 +00:00 (Migrated from github.com)
Copy Link
Copy Source

This PR doesn't upgrade components to be Glimmer components yet. It's already big enough as it is.

@galfert I'm not sure if you saw my chat message, but when you look at the tests you can find one helper failing (due to some kind of observer issues), and that one failure also seems to break the entire run loop of the Ember app when using it normally, thus freezing the app in place when it hits (which is when you look at contribution details). Hence the WIP.

I also removed all code relating to proposals on the way, as we're not using them at all at the moment. Thus, dragging the code along through big upgrades like this one didn't make sense to me. We can always retrieve it from Git whenever we need it again.

This PR doesn't upgrade components to be Glimmer components yet. It's already big enough as it is. @galfert I'm not sure if you saw my chat message, but when you look at the tests you can find one helper failing (due to some kind of observer issues), and that one failure also seems to break the entire run loop of the Ember app when using it normally, thus freezing the app in place when it hits (which is when you look at contribution details). Hence the WIP. I also removed all code relating to proposals on the way, as we're not using them at all at the moment. Thus, dragging the code along through big upgrades like this one didn't make sense to me. We can always retrieve it from Git whenever we need it again.
fsmanuel (Migrated from github.com) reviewed 2020-05-11 08:39:29 +00:00
galfert commented 2020-05-11 10:20:56 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Haha, now I wonder how it ever NOT caused an exception :) There was a typo in the property name in the function that removes the observer. I guess older Ember versions kept track of existing observers in a different way that didn't fail when trying to remove a non-existent observer.

Haha, now I wonder how it ever *NOT* caused an exception :) There was a typo in the property name in the function that removes the observer. I guess older Ember versions kept track of existing observers in a different way that didn't fail when trying to remove a non-existent observer.
😆 1
raucao commented 2020-05-11 10:37:01 +00:00 (Migrated from github.com)
Copy Link
Copy Source

wowitworked

![wowitworked](https://user-images.githubusercontent.com/842/81552626-18d80d80-9384-11ea-8ac2-81a9435d7baf.gif)
raucao commented 2020-05-11 10:38:05 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@galfert Thanks!

So this is ready to review/merge then.

@galfert Thanks! So this is ready to review/merge then.
galfert commented 2020-05-11 10:38:59 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I'm already reviewing it.

I'm already reviewing it.
🎉 1
galfert (Migrated from github.com) reviewed 2020-05-11 10:54:02 +00:00
galfert (Migrated from github.com) left a comment
Copy Link
Copy Source

Code looks good to me. Just left some minor comments.

But the kredits amount in the "Confirmed Contrubutions" list looks misaligned now:

kredits
Code looks good to me. Just left some minor comments. But the kredits amount in the "Confirmed Contrubutions" list looks misaligned now: <img width="616" alt="kredits" src="https://user-images.githubusercontent.com/843/81553971-4625bb00-9386-11ea-9527-dce066bf6646.png">
app/components/contribution-list/template.hbs
galfert (Migrated from github.com) commented 2020-05-11 10:29:18 +00:00
Copy Link
Copy Source

Did you remove the use of the contribution-status helper because of the bug or is it just not needed here anymore?

Did you remove the use of the `contribution-status` helper because of the bug or is it just not needed here anymore?
app/templates/dashboard.hbs
@@ -1,42 +1,44 @@
{{#if showIntroText}}
<div id="intro" class={{if showDetailsPane "with-details"}}>
{{#if this.showIntroText}}
galfert (Migrated from github.com) commented 2020-05-11 10:45:03 +00:00
Copy Link
Copy Source

I think the id attribute is not supposed to be prefixed with the @.

I think the `id` attribute is not supposed to be prefixed with the `@`.
codemods.log
galfert (Migrated from github.com) commented 2020-05-11 10:47:24 +00:00
Copy Link
Copy Source

This file shouldn't be committed. Probably needs a .gitignore rule.

This file shouldn't be committed. Probably needs a `.gitignore` rule.
galfert (Migrated from github.com) reviewed 2020-05-11 10:57:40 +00:00
app/components/add-contributor/template.hbs
@@ -1,55 +1,56 @@
<form {{action "submit" on="submit"}}>
<form onsubmit={{action "submit"}}>
galfert (Migrated from github.com) commented 2020-05-11 10:57:40 +00:00
Copy Link
Copy Source

No @ for id, placeholder and class, but one should be added to value.

No `@` for `id`, `placeholder` and `class`, but one should be added to `value`.
raucao (Migrated from github.com) reviewed 2020-05-11 11:02:30 +00:00
app/components/contribution-list/template.hbs
raucao (Migrated from github.com) commented 2020-05-11 11:02:30 +00:00
Copy Link
Copy Source

Yes, because of the bug. But this was an accidental commit. I just needed it locally to work on the rest.

Yes, because of the bug. But this was an accidental commit. I just needed it locally to work on the rest.
raucao commented 2020-05-11 11:18:17 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Thanks!

(I cannot reproduce the misalignment of the amounts.)

Thanks! (I cannot reproduce the misalignment of the amounts.)
galfert commented 2020-05-11 11:50:51 +00:00 (Migrated from github.com)
Copy Link
Copy Source

(I cannot reproduce the misalignment of the amounts.)

Looks ok here now, too. I think it was the missing contribution-status class.

> (I cannot reproduce the misalignment of the amounts.) Looks ok here now, too. I think it was the missing contribution-status class.
👍 1
Sign in to join this conversation.
Reviewers
No Reviewers
fsmanuel
galfert
raucao
Labels
Clear labels
bug
Something is not working
 dev environment
Config, builds, CI, deployment, etc.
 docs
Documentation
 duplicate
This issue or pull request already exists
 enhancement
Improving existing functionality
 feature
New functionality
 idea
Something to consider
 invalid
Not a bug
 kredits-1
Small contribution
 kredits-2
Medium contribution
 kredits-3
Large contribution
 question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
 ui/ux
User interface, process design, etc.
 wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/kredits-web#186
Delete Branch "chore/upgrade_ember_and_deps"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?