Upgrade sparkles components to glimmer #288
Conversation
mansona
force-pushed
the
upgrade
branch
4 times, most recently
from
8f5d8fe to
c7f3a7e
Compare
|
Conflicts ahead! |
mansona
force-pushed
the
upgrade
branch
2 times, most recently
from
a1ba310 to
dd03a7e
Compare
|
There seems to be some sort of bug that is preventing me to move any of the |
|
Looks like this has some conflicts @mansona |
|
@MelSumner I mention in my last comment that this is blocked by #301 so I'm going to wait until that is merged before I fix any conflicts 👍 I'll add a note to the main description too |
mansona
dismissed
RobbieTheWagner’s stale review
this PR has changed a lot since this review
| @@ -1,9 +1,10 @@ | |||
| import Component from '@ember/component'; | |||
| import layout from './template'; | |||
| import layout from 'ember-styleguide/templates/components/es-header-navbar-link'; | |||
There was a problem hiding this comment.
could we keep the relative import?
There was a problem hiding this comment.
I would prefer not to because none of the other components have it, we really should upgrade this to Glimmer after this is merged but it's not a blocker for release
I also moved this because of the bug we discussed with ember-angle-bracket-invocation-polyfill, we can't use nested angle bracket components any more so I needed to move the component 👍
| this.setupLinkBlurs(); | ||
| if (this.isDropdownOpen) { | ||
| // if it's open, let's make sure it can do some things | ||
| schedule('afterRender', this, function() { |
There was a problem hiding this comment.
this needs to go ASAP after the merge
There was a problem hiding this comment.
I am planning on creating an issue after this is merged to convert this component into a glimmer component and I will make sure there is a todo item to remove the runloop stuff too 👍
| } | ||
| </style> | ||
|
|
||
| <div class="layout mt-3"> |
There was a problem hiding this comment.
I don't really understand this change 🤔 Is the idea to move the piece of data somewhere?
There was a problem hiding this comment.
Just copying the explanation I had in a DM here:
Essentially the <ColourPallet /> component is now coming from upstream (Field Guide) and because of that we had to add some arguments to each of these entries. Instead of exploding the size of these components with a lot of duplicated arguments I thought it would be "simpler" to restructure the page, although I'm happy to revert this if it's a blocker and too complicated 👍
This includes an update with ember-cli-update so that we can start making use of Octane things