search

webark / ember-cli-styles

4 stars 2 forks source link

Improve `style-namespace` helper #9

Closed boris-petrov closed 2 years ago

boris-petrov commented 2 years ago

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 commented 2 years ago

So I think this looks good. The one thing is this is still reliant on https://github.com/webark/ember-cli-styles/blob/4ead8b009e1905a6d8d3469edc7595c57e97b448/packages/namespace/addon/utils/process-style-type.js#L18-L20

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

boris-petrov commented 2 years ago

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

webark commented 2 years ago

hahaha!! fair point

webark commented 2 years ago

@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 commented 2 years ago

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

boris-petrov commented 2 years ago

@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 commented 2 years ago

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

boris-petrov commented 2 years ago

Nope. Perhaps try adding them again?

webark commented 2 years ago

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

boris-petrov commented 2 years ago

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

webark commented 2 years ago

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

webark commented 2 years ago

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

https://github.com/webark/ember-cli-styles/blob/1a536fde4e340a4da27c93f98b9c0ceecf28d08f/packages/namespace/tests/acceptance/unique-component-paths-test.js#L11-L15 https://github.com/webark/ember-cli-styles/blob/1a536fde4e340a4da27c93f98b9c0ceecf28d08f/packages/namespace/tests/dummy/app/router.js#L35 https://github.com/webark/ember-cli-styles/blob/1a536fde4e340a4da27c93f98b9c0ceecf28d08f/packages/namespace/tests/dummy/app/unique-component-paths/template.hbs#L1 https://github.com/webark/ember-cli-styles/blob/1a536fde4e340a4da27c93f98b9c0ceecf28d08f/packages/namespace/tests/dummy/app/unique-component-paths/-components/test-component/template.hbs#L1 https://github.com/webark/ember-cli-styles/blob/1a536fde4e340a4da27c93f98b9c0ceecf28d08f/packages/namespace/tests/dummy/app/unique-component-paths/-components/test-component/styles.scss#L2

this is the path of how that test is setup.

boris-petrov commented 2 years ago

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

webark commented 2 years ago

@boris-petrov this has been released. 

 - ember-cli-styles-colocation@1.0.0-alpha.4
 - ember-cli-styles-namespace@1.0.0-alpha.7
 - ember-cli-styles-preprocessor@1.0.0-alpha.4
boris-petrov commented 2 years ago

Thank you!

webark commented 2 years ago

Thank you!