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

Improve style-namespace helper #9

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Improve style-namespace helper #9

merged 1 commit into from
Feb 1, 2022

Conversation

boris-petrov
Copy link
Contributor

@boris-petrov boris-petrov commented Jan 14, 2022

Fixes #5

cc @webark

As I mentioned in #5 this fixes the issue for me. Please tell me if this is completely wrong and that it should not be used like that.

@webark
Copy link
Owner

webark commented Jan 14, 2022

So I think this looks good. The one thing is this is still reliant on

owner
.lookup('container-debug-adapter:main')
.catalogEntriesByType(extention)

I'm not sure what the long-term support, or support in general, is for that API.

@boris-petrov
Copy link
Contributor Author

Well, if it works on Ember 4 right now, it's going to work until at least Ember 5. 😄 We can think what to do when Ember 5 starts approaching. 😃

@webark
Copy link
Owner

webark commented Jan 14, 2022

hahaha!! fair point

@webark
Copy link
Owner

webark commented Jan 30, 2022

@boris-petrov Left a couple of comments.

Also, could we switch the commits to be conventional commits, and reference the issue number where applicable?

I haven't added a CONTRIBUTING.md yet, nor something like husky. But want to keep that format.

@webark
Copy link
Owner

webark commented Jan 30, 2022

Also, let's add this use case to the acceptance tests.

@boris-petrov
Copy link
Contributor Author

@webark - I changed the commit messages, is this what you mean by "conventional commits" and reference the issue?

I don't see the comments that you've added, where is that?

Also, can you point me where exactly I should add a test? I see only "tests" in the dummy app but I'm not sure how that whole thing works.

@webark
Copy link
Owner

webark commented Jan 31, 2022

can you not see the three comments attached to lines of the changes in the PR?

@boris-petrov
Copy link
Contributor Author

Nope. Perhaps try adding them again?

@webark
Copy link
Owner

webark commented Jan 31, 2022

weird. I just "assigned" this to you.. can you see the review comments now?

@boris-petrov
Copy link
Contributor Author

No... can you send me a screenshot of the comments? 😄

@webark webark requested review from webark and removed request for webark February 1, 2022 00:40
@webark
Copy link
Owner

webark commented Feb 1, 2022

🤦 I had forgotten to hit submit. sorry @boris-petrov

@webark
Copy link
Owner

webark commented Feb 1, 2022

And for the test, you can set up a new route in the dummy app, use the differing component name, and then set up an acceptance test to verify it.

Cause that process isn't laid out currently, you could add it to the unique file name test for now, and I can split it off later..

test('base rule followed', async function (assert) {
await visit(`/unique-component-paths`);
assert.strictEqual(this.styleFor('h1').color, 'rgb(0, 0, 14)');
});

this.route('unique-component-paths');

<UniqueComponentPaths::-components::TestComponent></UniqueComponentPaths::-components::TestComponent>


this is the path of how that test is setup.

@boris-petrov
Copy link
Contributor Author

@webark - I believe I did everything you requested! Thanks for the links for the tests - I wouldn't have figured it out on my own.

I've added a passing test and I've split the other changes in different PRs.

@boris-petrov boris-petrov requested a review from webark February 1, 2022 20:32
@webark webark merged commit 0a9f129 into webark:main Feb 1, 2022
@webark
Copy link
Owner

webark commented Feb 11, 2022

@boris-petrov this has been released.

@boris-petrov
Copy link
Contributor Author

Thank you!

1 similar comment
@webark
Copy link
Owner

webark commented Feb 11, 2022

Thank you!

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.

Generate a manifest
2 participants