-
Notifications
You must be signed in to change notification settings - Fork 50
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
Modernize addon #99
base: master
Are you sure you want to change the base?
Modernize addon #99
Conversation
I see now that there's already #88 open, which never landed. Even if this doesn't land, that's fine with me -- we may just publish the modernized version in that case under |
@alexlafroscia maybe we should hand over maintenance of this to movable? I don't believe it is used anymore in any of Yapp's apps, and most maintenance has been done by former Yapper @raycohen. If movable ink is actively using this repo, you would likely be in a better position to support it than us. Thoughts? |
That’s fine with me, I don’t mind taking over maintenance! Would be nice to avoid having our own fork and publishing under our namespace. |
@raycohen WDYT? |
@lukemelia I am fine handing over maintenance to movable. That said I have a few thoughts- First, I think it would be worth addressing the yarn v2 compatibility issue without releasing breaking changes and a new version, if possible. This is a utility addon apparently in use by a decent number of consumers, so keeping compatibility seems more valuable than moving everything to the latest hotness. That said we've been pretty stable on adding new features lately so maybe this isn't a big deal. Second, I think there's room for more than one radio button addon and and one that reimagines the API from the ground up using the newest ember facilities could be worth having. I'd even put an "alternatives" section near the top of this readme to raise awareness of an alternative. Seeing as @alexlafroscia put the energy into this modernization perhaps that's an area he'd be interested in exploring. As he mentioned, this API has the issue with attribute forwarding and two different elements that might want to receive forwarded attributes. Maybe theres an API that yields contextual components or uses other new features in ember that came to be after this API was conceived is worth exploring. All that said, and either way it goes with this PR, I am fine with handing over maintenance and decision making. |
@alexlafroscia I've added you as a maintainer in npm and invited you as an admin collaborator to this repository. Let me know if there's anything else you need on my end for the maintainership transition. Thanks and good luck! On the merits of the PR, I think Ray's notes are sound and worth considering. |
Hi guys, I don't know what the status is of this conversation or if there already are any new initiatives, but I decided to take a stab at devising an alternative nonetheless. Any kind of feedback would be awesome! Feel free to comment on the pull request. |
Is there a way we can build @bertdeblock proposal into @alexlafroscia's work? What are the next steps for this? |
I started an initial implementation a while back just for fun. |
This PR updates the add-on to modern dependencies and, in some cases, modern patterns.
It does not update to use full-blown Octane pattern because of the
radio-button
component's wrapping of both aninput
andlabel
element in some cases. Using attribute forwarding would simplify a bunch of the logic around ARIA attributes,disabled
,required
etc. which would be really nice. However, there's no way to forward some of the attribute to theinput
and others to thelabel
.The motivation for this work was trying to upgrade my app at work to Yarn V2. The
ember-cli-htmlbars
version previously used here depended on anember-cli-version-checker
version that does not play nicely with Yarn PNP.I would cut a new major release after landing this, as
sendAction
) was removed