Skip to content

Conversation

@getglad
Copy link

@getglad getglad commented Apr 13, 2016

See getglad/inky@25c4441

Partially resolves #308

Happy to answer any questions

Grid adjustments related to getglad/inky@25c4441
@rafibomb
Copy link
Member

Thanks for the PR! How does adding the extra class solve the problem? Is the redundancy of the classes the solution? We'd love to understand this better!

@getglad
Copy link
Author

getglad commented Apr 20, 2016

The real improvement is in Inky (foundation/inky#34) - realizing now that I didn't link directly to that PR.

The naming convention may not be the best/clearest, but I didn't want to submit a PR to Inky that could end up being a breaking change to folks' existing templates. So instead I created a second/similar element that would allow for the additional rows and avoid reusing element names.

This addition to the the grid file is just to keep the styles applied uniformly to both types of elements, so that a table with multiple rows inside of it style just like a table with only one row inside of it.

Update/Edit:
Okay - I see the confusion. I am having to copy/paste my changes onto Github (corporate network blocking my pushes), and messed two things up:

  1. I used column2 + columns2 in _grid.scss - that was an unnecessary redundancy from my POV, but matched the existing style of using both column and columns in the file.
  2. In my Inky PR, this line (https://github.com/zurb/inky/pull/34/files#diff-800f6c298551634ed54bfc8e9266f524R33) is adding columns instead of columns2. This was creating a weird combo of row/columns and row2/columns, when I had meant row/columns and row2/columns2.

So that actually made me realize 1 other thing. I can leave the Inky template change in to get the additional component (table + more than 1 row), but make the style names to be the same (styled as row + columns), which would nix the need for this change. (Maybe that is what you were getting at).

So, I need to do one of two things:

  1. Change this line (https://github.com/zurb/inky/pull/34/files#diff-c0f1ea57f183fcb7843f7275597a50f9R32) from var classes = ['row2']; to var classes = ['row']; and close this PR. Lose unique component class name, but, personally, I don't see that as a loss.
  2. Change this line (https://github.com/zurb/inky/pull/34/files#diff-800f6c298551634ed54bfc8e9266f524R33) from classes.push('columns'); to classes.push('columns2'); and keep this PR. Maintain unique component class name, but increase complexity.

I would lean towards doing # 1, but I know you service both, so maybe you have a preference / a different suggestion?

@rafibomb rafibomb modified the milestone: 2.2.0 Jun 10, 2016
@rafibomb rafibomb closed this Jul 8, 2016
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.

2 participants