modernize addon: moved from webuniversum to appuniversum, converted to glimmer components #4
Conversation
|
Hey Niels ! I tried to review but I seem to have issues running the addon in Loket / Toezicht / ... . I was suspecting an issue with the Ember version we use (3.20) so I tried it out on Loket 3.24. It worked at some point, then when I removed and re-installed the modules it broke again. I wasn't able to find what to update to fix it. |
addon/components/acmidm-login.hbs
Outdated
| @@ -0,0 +1,6 @@ | |||
| <AuButton @loading={{this.isAuthenticating}} @disabled={{this.isAuthenticating}} {{on "click" this.login}}> | |||
There was a problem hiding this comment.
Instead of using (and depending on) Appuniversum, it might be nice to keep these components styling agnostic?
I think this can easily be achieved by yielding state instead of outputting html. That way projects can decide which components to use.
Depending on appuniversum will also create an extra maintenance burden since it will need to be bumped from time to time (especially since it's a 0.x release).
There was a problem hiding this comment.
Most projects also seem to implement a "compact" version (which duplicates the logic) of this component for the main header. That would then no longer be needed either.
There was a problem hiding this comment.
I think that's a great idea, but would keep this for backwards compatibility. perhaps adding a deprecation warning in the constructor?
There was a problem hiding this comment.
For the data-table we took the approach of a styling agnostic addon and another addon that extends it with Appuniversum, but that results in a heavier maintentance burden I must admit (and still makes it a breaking change for this addon atm).
I'm pro the deprecation warning in this PR. It paves the way to a styling agnostic version and thus a breaking release in the future. But I would implement that in a separate PR and not lump it on this one.
There was a problem hiding this comment.
I only see now that there is a PR #5 already 🙈
There was a problem hiding this comment.
Yes, I created a follow-up PR as a quick POC of what a styling agnostic version could look like. I don't think the deprecation warnings should be added here since there are no alternatives yet, but I can add them to that PR as well.
addon/components/acmidm-logout.hbs
Outdated
| @@ -1,5 +1,7 @@ | |||
| <button ...attributes {{on "click" this.logout}}> | |||
There was a problem hiding this comment.
not sure if this component is really needed since it's just a button wrapper for ember-simple-auth's session invalidation logic (which is not really related to acm/idm either)?
There was a problem hiding this comment.
(I just wanted to get the conversation started. If this is a wanted change we can deprecate the component first in a separate PR)
These components are styling independant so apps can use them if the default button components don't cover the needed use cases.
odd, dieter reported this as well. should look into it |
Also encountering this error on the |
|
Found a bug ticket reporting the same error Going to push some additional changes and test those using Github dependency instead of NPM linking. |
Refactor to native class fails because of deprecation warning on computed properties baseUrl/scope/redirectUri being overwritten.
Create Login and Switch provider components
did some basic testing in the dummy app and in GN (switching was only tested in the dummy app).
fixes #3