Skip to content

Conversation

@rlueder
Copy link

@rlueder rlueder commented Dec 8, 2016

What's in this PR?

This PR is taking care of most of the refactoring needs described in #672

The approach I'm taking is (somewhat) documented here: Code Corps SASS Ideas and Discussion

References

Progress on: #672

@joshsmith
Copy link
Contributor

Wow, this is a lot of work. Okay, I need time to sit down and go through this. Looks like there are conflicts due to my changes now too @rlueder, and some failing tests.

Would you be able to fix conflicts/tests and then I can make sure I sit down to review this and understand it fully.

@WenInCode
Copy link
Contributor

Hey @rlueder - I saw that you mentioned you were a bit confused about decoupling the tests from css. It so happens that your tests are failing for that very reason. In this PR, you've updated some classes to follow the BEM style guide. Some of the page objects (used in tests to represent/select from pages and/or components) used those css classes as selectors. Because those css classes no longer exist, they are not returning the expected values, and in result the tests are failing.

Going forward we want to use data-test-selector attributes on elements that need to be accessed in tests. This way we can make descriptive selectors that won't change during styling changes.

A good example of this failing test. It is failing because this class name was replaced. The page object for that component scopes by that class. For this reason, the test fails as it cannot find the element. The solution to this is to apply a new data-test-selector attribute to the corresponding component, and scoping by that in the page object.

To add a data- attribute to the component you'll need to follow the guides

basically you'll be doing the following:

{{!-- app/templates/start/interests.hbs --}}
{{categories-list categories=model data-test-selector="Categories List"}}

and in the component

// app/components/categories-list.js
...
export default Component.extend({
  attributeBindings: ['data-test-selector'],  // <-- you'll need to add the attributeBindings
  classNames: ['start__interests'],
...

Now that the data-test-selector is set up we can scope by it in the page object

// tests/pages/components/categories-list.js
...
export default {
  scope: '[data-test-selector="Categories List"]',
...

This is just an example of one of the changes, but I hope it sheds some light on what was meant by the decoupling of tests from css. Let me know if you have any questions, I'd be more than happy to help.

@joshsmith
Copy link
Contributor

@WenInCode is this documented in our tasks about how to do this, or somewhere else? If not, think good to add so those tasks can be legitimately labeled good for new contributors.

@WenInCode
Copy link
Contributor

@joshsmith It is not currently, I just wrote it up last night for this. I can add it to #698 though. Might be good to mention somewhere in contributing docs as well for new folks contributing.

@joshsmith
Copy link
Contributor

@WenInCode agreed

@rlueder rlueder self-assigned this Dec 17, 2016
@rlueder rlueder added this to the Refactor CSS milestone Dec 17, 2016
@joshsmith joshsmith assigned joshsmith and unassigned rlueder Feb 10, 2017
@joshsmith joshsmith force-pushed the 672_use-BEM-for-styles branch 4 times, most recently from c7d63b5 to 0cddf0f Compare February 10, 2017 05:02
@joshsmith joshsmith force-pushed the 672_use-BEM-for-styles branch from 0cddf0f to 2c7df57 Compare February 10, 2017 05:10
Copy link
Contributor

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally fixed issues I caused by waiting so long to review and reviewed this. Thanks @rlueder for all your hard work here.

@joshsmith joshsmith force-pushed the 672_use-BEM-for-styles branch from 91c35de to 10685e6 Compare February 10, 2017 05:41
@joshsmith joshsmith merged commit ceaa2e1 into develop Feb 10, 2017
@joshsmith joshsmith deleted the 672_use-BEM-for-styles branch February 10, 2017 05:48
@joshsmith
Copy link
Contributor

🙌 @rlueder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants