Skip to content
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

Offset and ordering not following column naming convention #54

Closed
jlafitte opened this issue Apr 25, 2018 · 7 comments
Closed

Offset and ordering not following column naming convention #54

jlafitte opened this issue Apr 25, 2018 · 7 comments

Comments

@jlafitte
Copy link

So the column naming convention is:
.col-<breakpoint>-<n>

But the order and offset are:
.order-<n>-<breakpoint>
.offset-<n>-<breakpoint>

This creates confusion for us. We think they should be:
.order-<breakpoint>-<n>
.offset-<breakpoint>-<n>

I was just going to "fix" it in my fork, but then your nice documentation would no longer apply. What do you think about this issue? Any chance you may reconsider the naming for these?

@marcorieser
Copy link
Contributor

Seems like i mixed up something with my pull request bringing in this feature. Why don't you fix the docs too and make a pull request?

@jlafitte
Copy link
Author

I wouldn't blame you as it looks like you were following the convention for order, but it seems to make sense that there is a global common naming convention. SASS isn't really my specialty (my fork is a LESS version of it) but I will try to fix it and do a pull request when I can make some time.

@marcorieser
Copy link
Contributor

@jlafitte i pushed the fix as a pull request.

@leejordan
Copy link
Owner

I think this is a fair comment. I will look to merge marcorieser's pull request. The problem is that now I am used to using the classes in this way so although your suggested change is good for consistency it means I'll have to get used to the new syntax :)

@leejordan
Copy link
Owner

I still plan to merge this, I just need to find some time to look at it properly.

@sntran
Copy link
Contributor

sntran commented Oct 11, 2018

@leejordan Did you have a chance to look at this minor change? Consistency is good.

@leejordan
Copy link
Owner

It's merged and in the latest release 2.0.4

Thanks!

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

No branches or pull requests

4 participants