-
Notifications
You must be signed in to change notification settings - Fork 4
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
Convert to a V2 addon #72
Comments
Yes definitely 👍 |
I cant believe I missed this but https://github.com/rajasegar/ember-x-tabs/blob/master/tests/dummy/app/templates/index.hbs#L13 should be |
At this point I have a V1 addon reformatted as a mono repo using yarn workspaces. https://github.com/bgantzler/ember-x-tabs/tree/v2 I have tried to update the github actions, but most like I have broken some of them and you may need to fix after. At this point in the guide we are suppose to considering splitting the demo app from the test-app. (I would suggest this). Are you ok with me doing that? (paused until I get an answer) |
Yeah I am definitely OK with that 👍 |
I agree, work got in the way, havent gotten back to it, will as soon as I can, half way done |
Pretty close. Have to figure out how to do the conditional import of the themes under V2 Think more your CI, release-it, etc will be broken. Will have to rework that. Think you might have to do as you have added a couple things I am unfamiliar with. The EQ helper, that really isnt part of the add on is it? Meant really for the test-app correct? Looks like template-only components dont work yet in V2 so had to create the empty files, linting may need to turn that that specific lint off To publish, you will run npm publish in the addon directory. |
Heres the current branch, need to cleanup the EQ helper (question above) so not ready yet https://github.com/bgantzler/ember-x-tabs/tree/v2 You have three packages now. The addon, test-app (the tests) and docs (the demo app). Thats why your CI will need adjusted. To run linting in all three, your demo app can have tests (specific to the demo app) so could write and run them, etc. I think some thinks need to be moved from docs (release.config.js, renovate.js for example), semantic-release probably shouldnt be in the docs/package.json, hope you can figure out where those things should be. |
https://discord.com/channels/480462759797063690/955484238713602099 May have to have users do the css imports on their own, follow the discussion |
I have the themes working, but yes the users will have to specify the imports for the themes they want to include. It will be a breaking change as themes will NOT be included by default. There is a all-themes file provided so they dont have to list every one. This is similar to the way ember-pikaday requires users to now include the css https://github.com/adopted-ember-addons/ember-pikaday#styles The should be now a fully V2 compliant addon. Things that now need to be fixes at the github actions, deploying the demo app, etc. |
The branch in my fork is https://github.com/bgantzler/ember-x-tabs/tree/v2 not sure if want a PR against master, or if you want to create a new banch off master and have me make the PR against that because while the addon is converted, there are still updates needed to the github actions. Let me know how you would like to proceed from here. |
I think creating a new branch off master and making the PR against it sounds like a safer and better option for me, once we are fully confident of the changes then we can merge with master |
You already have a V2 branch, I dont think I should use that as that is not what I used as my base and would have to deal with a lot of conflicts. You will need to create that branch and let me know what it is. Ill rebase my code and do a PR. |
Need a few things to finish this as a V2 addon. Please read the issue |
You can use the v3 branch |
Doing some MirageJS work, will get back to this as soon as Im able |
Working on this again. If you could not update any more dependancies (It causes conflicts in the package.json since its quite different) I would appreciate it. It looks like you are using pnpm recently, will have to work through that as I have not used before, so will use https://github.com/NullVoxPopuli/ember-resources/blob/main/.github/workflows/ci.yml as a guide since its in the V2 format if thats ok. Will take me a little longer to convert this, had it running under yarn |
Yeah that's fine, no problem, I won't update any dependencies |
Looking at the addon because I think I broke setting an active tab other than the first tab on initial display and see I completely forgot about finishing this. You still around to update PRs? Ill finish this and fix the issue. So sorry for getting lost |
Just putting this as a placeholder. Would like to convert this addon to the V2 format unless you wanted to do it.
Let me know if I should proceed
The text was updated successfully, but these errors were encountered: