monterail / guidelines

[DEPRECATED] We are Ruby on Rails experts from Poland. Think hussars. Solid & winged. These are our guidelines.
71 stars 17 forks source link

Characters per line #248

Open Ruto8 opened 8 years ago

Ruto8 commented 8 years ago

Some time ago, we agreed that 80 characters are insuficient and started to use 100 characters per line as our guideline. I was looking for that in our guidelines, and when I couldn't find that, surprised I was.

Before making a pull-request with that, I think we need a discussion about template files. As I noticed, we're often exceeding 100 characters per line, becuase of BEM classes. As an example:

  span.ui-typo__serif-bigger.ui-posts__dropdown-header.ui-posts__dropdown-bg-hack.post-list__state

I can think of 2 solutions

  1. Classes should be refactored, to be less than 100 characters (don't forget indentation)
  2. We shouldn't care about 100 characters in template files.

Is there anything we can do about it or how should we proceed? Should we add this guideline at all?

kubakrzempek commented 8 years ago

There are three solutions that poped into my head

  1. Merge attributes to be inline with the guidelines
span.ui-typo__serif-bigger.ui-posts__dropdown-header(
  class="ui-posts__dropdown-bg-hack post-list__state"
)
  1. Multline attributes be inline with the guidelines
span(
  class="ui-posts__dropdown-bg-hack post-list__state\
    ui-typo__serif-bigger ui-posts__dropdown-header"
)
  1. Don't get over-excited about it and be reasonable. if there were 200 chars probably something should be done, if there were like 110, meeh.
damaon commented 8 years ago

@Ruto8 We can simply tolerate too long line if it contains one word. I can simple checker for circleCi but it's required to extract from git only lines modified since some date to avoid too big changes.

damaon commented 8 years ago

git log --pretty=format: --name-only --since="2016-01-01" | sort | uniq

shentao commented 8 years ago

Imho we shouldn't be overengineering this. We could however think about creating more readable class names and get rid of redundant stuff. Like the hack thing in the example. And ui-typo__serif-bigger which is also not correct BEM. Additionally SMACSS could be our solution here. If the element is so complex that it requires so many classes – maybe it should be converted to a new component? Sure, there are perks of combining elements from smaller classes, so we avoid creating new ones. But the moment we start to reuse this combinations this might become painful and hard to manage. @Ruto8 @kubakrzempek @Machiaweliczny WDYT?

PS I also don't like the multiline syntax for class="" attribute.