-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Addon native classes #814
base: master
Are you sure you want to change the base?
Addon native classes #814
Conversation
@maxwondercorn , @rwwagner90. Do you have a plan or currently work in progress to convert this to glimmer? |
@vstefanovic97 we would love the help! I think either way is fine. |
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 think we need to take another look at what should be @tracked
and make sure all the things are updating as we expect. Alternatively, we could put back some of the old computeds.
Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
Co-authored-by: Robert Wagner <rwwagner90@gmail.com>
Can not track properties until Column class is converted to native class
Added computed back. Most properties are on the class which still need to be converted to native classes |
@maxwondercorn I totally forgot about this PR, sorry about that! Where did we leave off? |
Decorators (
tagName
,classNamesBindings
, etc) remain because outer HTML breaks demo app/tests due to the extradivs
. Using@tagName('')
to remove them causes other errors.I believe everything needs to be converted to glimmer to finish the cleanup.
addObserver
,removeObserver
are used instead of the@observes
decorator because it's not glimmer compatible