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

[Grid] made the grid adjustable #80

Merged
merged 7 commits into from Jun 15, 2016

Conversation

Projects
None yet
5 participants
@retober
Contributor

retober commented May 26, 2016

I made the grid adjustable. Please have a look. Now it is possible to adjust the number of grid columns, the spacing between the columns and the initial grid width. You are also able to customize the breakpoints.

Closes #76

@retober retober changed the title from made the grid adjustable to [Grid] made the grid adjustable May 26, 2016

@catchamonkey

This comment has been minimized.

Show comment
Hide comment
@catchamonkey

catchamonkey May 26, 2016

Member

Hey @retober thanks for the PR.
Can you sort out the indentation and formatting of your changes so it's easier to review please?

Member

catchamonkey commented May 26, 2016

Hey @retober thanks for the PR.
Can you sort out the indentation and formatting of your changes so it's easier to review please?

@catchamonkey catchamonkey self-assigned this May 26, 2016

@catchamonkey catchamonkey added this to the 4.0 milestone May 26, 2016

@retober

This comment has been minimized.

Show comment
Hide comment
@retober

retober May 26, 2016

Contributor

I'm not used to this kind of formatting but i tried to fix the indentation.

Contributor

retober commented May 26, 2016

I'm not used to this kind of formatting but i tried to fix the indentation.

Show outdated Hide outdated scss/_grid.scss
.grid--offset-9 { margin-left:75%; }
.grid--offset-10 { margin-left:83.333%; }
.grid--offset-11 { margin-left:91.667%; }
@for $i from 1 through $grid-columns - 1 { .grid--offset-#{$i} { margin-left: 100% / $grid-columns *$i; } }

This comment has been minimized.

@catchamonkey
@catchamonkey
@catchamonkey

This comment has been minimized.

Show comment
Hide comment
@catchamonkey

catchamonkey May 27, 2016

Member

Looks good so far @retober

We will need to have an update to the docs too please.
Need to explain the process of building your own version if you want to override the grid quantity etc.
It might be best to break out the variables into a separate SCSS file, as that way to build your own you can just include your own setup var file, include the amazium files and generate your end result.
Do you see what I mean?

Member

catchamonkey commented May 27, 2016

Looks good so far @retober

We will need to have an update to the docs too please.
Need to explain the process of building your own version if you want to override the grid quantity etc.
It might be best to break out the variables into a separate SCSS file, as that way to build your own you can just include your own setup var file, include the amazium files and generate your end result.
Do you see what I mean?

@retober

This comment has been minimized.

Show comment
Hide comment
@retober

retober May 27, 2016

Contributor

I agree, _variables.scss would be the best place to put the adjustable variables. What do you mean when you said that we need to update the docs. Which file do you mean ? Isn't amazium.co.uk the documentation ? I'm not a native speaker so i guess it is not advisable to let me write the documentation.

Contributor

retober commented May 27, 2016

I agree, _variables.scss would be the best place to put the adjustable variables. What do you mean when you said that we need to update the docs. Which file do you mean ? Isn't amazium.co.uk the documentation ? I'm not a native speaker so i guess it is not advisable to let me write the documentation.

@catchamonkey

This comment has been minimized.

Show comment
Hide comment
@catchamonkey

catchamonkey May 27, 2016

Member

@retober yes that's right, the amazium site covers all the usage and standard installation.
But the README in this repo also covers the basics, and think it would be best something relating to building your own column/gutter version in there.
Nothing wrong with your language at all, give it a go, we can always tweak it. But as this is your feature and approach it would be good to have you explain how you see it working.

Cheers 👍

Member

catchamonkey commented May 27, 2016

@retober yes that's right, the amazium site covers all the usage and standard installation.
But the README in this repo also covers the basics, and think it would be best something relating to building your own column/gutter version in there.
Nothing wrong with your language at all, give it a go, we can always tweak it. But as this is your feature and approach it would be good to have you explain how you see it working.

Cheers 👍

@retober

This comment has been minimized.

Show comment
Hide comment
@retober

retober Jun 1, 2016

Contributor

I'm writing a small documentation of my feature. There still seem to be some problems. And i have a question - do i have to add .grid now to every .grid-X or why does this class exist ?

Contributor

retober commented Jun 1, 2016

I'm writing a small documentation of my feature. There still seem to be some problems. And i have a question - do i have to add .grid now to every .grid-X or why does this class exist ?

@MikeBallan

This comment has been minimized.

Show comment
Hide comment
@MikeBallan

MikeBallan Jun 1, 2016

Member

@retober You will only use .grid when you want all the columns in your row to be the same width. If you want a specified width you would then use .grid-6. Hope that makes sense.

Member

MikeBallan commented Jun 1, 2016

@retober You will only use .grid when you want all the columns in your row to be the same width. If you want a specified width you would then use .grid-6. Hope that makes sense.

@MikeBallan

This comment has been minimized.

Show comment
Hide comment
@MikeBallan
Member

MikeBallan commented Jun 1, 2016

image

@retober

This comment has been minimized.

Show comment
Hide comment
@retober

retober Jun 1, 2016

Contributor

I'm just getting familiar with flexbox but now that i understand most of it - i like it 👍 Maybe .grid should be called .grid-auto then to prevent further confusion.

Contributor

retober commented Jun 1, 2016

I'm just getting familiar with flexbox but now that i understand most of it - i like it 👍 Maybe .grid should be called .grid-auto then to prevent further confusion.

@MikeBallan

This comment has been minimized.

Show comment
Hide comment
@MikeBallan

MikeBallan Jun 1, 2016

Member

Could do but grid is shorter to type :)

Member

MikeBallan commented Jun 1, 2016

Could do but grid is shorter to type :)

@retober

This comment has been minimized.

Show comment
Hide comment
@retober

retober Jun 1, 2016

Contributor

I wrote a short documentation of the new feature and fixed some errors. I tested it and it works fine now but somebody else should have a look at it and test it thoroughly.

Contributor

retober commented Jun 1, 2016

I wrote a short documentation of the new feature and fixed some errors. I tested it and it works fine now but somebody else should have a look at it and test it thoroughly.

@p2or

This comment has been minimized.

Show comment
Hide comment
@p2or

p2or Jun 1, 2016

Thanks guys. Good job! What about calling it .grid-x instead of .grid?

p2or commented Jun 1, 2016

Thanks guys. Good job! What about calling it .grid-x instead of .grid?

@catchamonkey

This comment has been minimized.

Show comment
Hide comment
@catchamonkey

catchamonkey Jun 1, 2016

Member

For me it's the default grid, without a size specific override (1-12), so just .grid makes sense.
Anything else would need to clarify it, which .grid-x doesn't do. Unless i've misunderstood what you meant with that one @p2or ?

Member

catchamonkey commented Jun 1, 2016

For me it's the default grid, without a size specific override (1-12), so just .grid makes sense.
Anything else would need to clarify it, which .grid-x doesn't do. Unless i've misunderstood what you meant with that one @p2or ?

@test2k

This comment has been minimized.

Show comment
Hide comment
@test2k

test2k Jun 1, 2016

Well done my friend! Keep on rockin' in the Free World!

test2k commented Jun 1, 2016

Well done my friend! Keep on rockin' in the Free World!

@catchamonkey

This comment has been minimized.

Show comment
Hide comment
@catchamonkey

catchamonkey Jun 2, 2016

Member

Thanks for your work here @retober looking good!

I'll pull down and do some testing against various projects.
Docs look fine too so nice one there.

/cc @MikeBallan

Member

catchamonkey commented Jun 2, 2016

Thanks for your work here @retober looking good!

I'll pull down and do some testing against various projects.
Docs look fine too so nice one there.

/cc @MikeBallan

@retober retober closed this Jun 2, 2016

@retober retober reopened this Jun 2, 2016

@p2or

This comment has been minimized.

Show comment
Hide comment
@p2or

p2or Jun 2, 2016

@catchamonkey It's more about consistency. I agree that .grid is short and easy to write but I think having the same length of grid class names leads into a clean design. Also .grid-x or grid-n indicates that the class is supplied from the framework, it's more unique in a larger scope, but especially it implies that there is something automatic going on which is easier to understand (IMO).

p2or commented Jun 2, 2016

@catchamonkey It's more about consistency. I agree that .grid is short and easy to write but I think having the same length of grid class names leads into a clean design. Also .grid-x or grid-n indicates that the class is supplied from the framework, it's more unique in a larger scope, but especially it implies that there is something automatic going on which is easier to understand (IMO).

@catchamonkey

This comment has been minimized.

Show comment
Hide comment
@catchamonkey

catchamonkey Jun 2, 2016

Member

@p2or my issue with .grid-x is that it doesn't add any clarity, as the x is as (possibly more) ambiguous as just .grid on it's own.

In terms of ensuring they are the same number of characters, that's kind of a side point, don't think that has ever been an issue in code clarity/cleanliness :)

Member

catchamonkey commented Jun 2, 2016

@p2or my issue with .grid-x is that it doesn't add any clarity, as the x is as (possibly more) ambiguous as just .grid on it's own.

In terms of ensuring they are the same number of characters, that's kind of a side point, don't think that has ever been an issue in code clarity/cleanliness :)

@MikeBallan

This comment has been minimized.

Show comment
Hide comment
@MikeBallan

MikeBallan Jun 15, 2016

Member

Had a chance to test this and looks great. Thanks for the PR!

Member

MikeBallan commented Jun 15, 2016

Had a chance to test this and looks great. Thanks for the PR!

@MikeBallan MikeBallan merged commit 0cf6ade into OwlyStuff:master Jun 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment