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

Convert to a V2 addon #72

Open
cah-brian-gantzler opened this issue Mar 8, 2022 · 18 comments
Open

Convert to a V2 addon #72

cah-brian-gantzler opened this issue Mar 8, 2022 · 18 comments

Comments

@cah-brian-gantzler
Copy link
Collaborator

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

@rajasegar
Copy link
Owner

Yes definitely 👍

@cah-brian-gantzler
Copy link
Collaborator Author

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 {{hash not (hash on all the lines . I found this while converting to V2. Since V2 requires moving all the files so not going to do a PR to fix it, will fix it in V2 if thats ok.

@cah-brian-gantzler
Copy link
Collaborator Author

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)

@rajasegar
Copy link
Owner

Yeah I am definitely OK with that 👍

@cah-brian-gantzler
Copy link
Collaborator Author

I agree, work got in the way, havent gotten back to it, will as soon as I can, half way done

@cah-brian-gantzler
Copy link
Collaborator Author

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.

@cah-brian-gantzler
Copy link
Collaborator Author

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.

@cah-brian-gantzler
Copy link
Collaborator Author

https://discord.com/channels/480462759797063690/955484238713602099 May have to have users do the css imports on their own, follow the discussion

@cah-brian-gantzler
Copy link
Collaborator Author

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.

@cah-brian-gantzler
Copy link
Collaborator Author

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.

@rajasegar
Copy link
Owner

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

@cah-brian-gantzler
Copy link
Collaborator Author

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.

@cah-brian-gantzler
Copy link
Collaborator Author

Need a few things to finish this as a V2 addon. Please read the issue

@rajasegar
Copy link
Owner

You can use the v3 branch
https://github.com/rajasegar/ember-x-tabs/tree/v3

@cah-brian-gantzler
Copy link
Collaborator Author

Doing some MirageJS work, will get back to this as soon as Im able

@cah-brian-gantzler
Copy link
Collaborator Author

cah-brian-gantzler commented Apr 26, 2022

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

@rajasegar
Copy link
Owner

Yeah that's fine, no problem, I won't update any dependencies

@cah-brian-gantzler
Copy link
Collaborator Author

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

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

No branches or pull requests

2 participants