Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

alexlafroscia
Copy link
Collaborator

@alexlafroscia alexlafroscia commented Jan 25, 2020

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 an input and label 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 the input and others to the label.

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 an ember-cli-version-checker version that does not play nicely with Yarn PNP.

I would cut a new major release after landing this, as

  • Support for legacy-style actions (sendAction) was removed
  • Testing is no longer done against any Ember 2.X versions

@alexlafroscia
Copy link
Collaborator Author

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 @movable/ember-radio-button or something if people want to use the modernized version.

@lukemelia
Copy link
Contributor

@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?

@alexlafroscia
Copy link
Collaborator Author

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.

@lukemelia
Copy link
Contributor

@raycohen WDYT?

@raycohen
Copy link
Contributor

@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.

@lukemelia
Copy link
Contributor

@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.

@bertdeblock
Copy link

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.

@allthesignals
Copy link

Is there a way we can build @bertdeblock proposal into @alexlafroscia's work? What are the next steps for this?

@bertdeblock
Copy link

I started an initial implementation a while back just for fun.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants