Dummy app updates #788
Dummy app updates #788
Conversation
|
@maxwondercorn is this still WIP? |
|
Ye still WIP. I've converted the dummy app to native classes and still have a couple issues to resolve. |
|
@maxwondercorn I just merged #757 which unfortunately caused some conflicts here. Sorry about that! |
This reverts commit a5347c3.
|
@rwwagner90 re: the conflicts above. I've already removed all the dummy app computed's with native getters or After this commit is merged, you can review and merge the PR. The Also, the new ember |
…ght-table into dummy-app-updates
…#791) * Fix implicit `this` issues * Update runloop imports * Use Object.assign
There was a problem hiding this comment.
@maxwondercorn looks like a lot of font-awesome changes. We should put all that back to how it was, as I updated font-awesome to work already.
It seems like maybe there were some rebasing issues, as I already fixed several of these things and some got changed back.
| @@ -7,9 +7,30 @@ | |||
| <p>{{@row.bio}}</p> | |||
|
|
|||
| <div class="user-actions"> | |||
| <a class="fa fa-envelope" href="mailto:{{@row.email}}"></a> | |||
| <a class="fa fa-facebook" href="#"></a> | |||
| <a class="fa fa-twitter" href="#"></a> | |||
There was a problem hiding this comment.
Why did we remove font-awesome here? I had updated font-awesome, so we should be able to still use it.
There was a problem hiding this comment.
New addon is official font-awesome add-on, not a community add-on. It does not support using class names. You need to use their FaIcon component or the CDN.
I believe you can create a "kit' which is mini CDN but that requires managing something through their servers. Long term we should try to remove the dependency. Everyone using ELT and cuurent addon have both both addons in their project.
There was a problem hiding this comment.
@maxwondercorn yes, I know it is an official font-awesome addon, I updated to it. The class names should still work. I converted a bunch of asc and desc to up and down and they worked.
There was a problem hiding this comment.
That's interesting. I had no icons displayed. Maybe I still had asc/desc instead of up/down in the names but none of the others were showing either. IIRC my main app had the same issue and I switched to the fa component.
I'll pull all the changes back in and will undo changes as needed
| {{#if this.hasSelection}} | ||
| <div | ||
| class="table-action fa fa-check-square" |
There was a problem hiding this comment.
Same with all the fa instances in this file. They should still work and we shouldn't change them.
Correct sorting icon names
|
Hopefully the final pass. Rebased and reverted font-awesome changes. I still do not see icons in app but this can be addressed later. All app examples work. |
|
@maxwondercorn I think some of the changes I made somehow made it into this PR. Perhaps a bad rebase or an accidental pull after rebasing before force pushing? Or perhaps an issue from me squashing and merging. Would you mind doing one final rebase and dropping the commits that should not be included in this PR please? |
|
Will look at it tomorrow. Very well could be a bad rebase. Wouldn’t be the first time. |
|
It does seem like font awesome icons are not showing up anymore, as you said. I don't know what changed since I opened my PR converting them. I will work on using the |
|
Glad I'm not too crazy re: the icons 😄. Yes it was a bad rebase. Looks like I pulled partial changes from other merged PR's. Too much work to fix. I just recreated the changes on the latest master and will close this PR. I'll have a new PR shortly |
|
Closing in preference of PR #793 |
No description provided.