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

Make Lost-Offset work more intuitively (reverse of current +/- behavior) #184

Closed
hudochenkov opened this issue Nov 2, 2015 · 33 comments
Closed

Comments

@hudochenkov
Copy link
Contributor

@hudochenkov hudochenkov commented Nov 2, 2015

In every grid systems offset always applied to the left side of the block, but in Lost it's applied to the right. Luckily we can apply offset to usual side by negative value, but it's counterintuitive.

What is the reason for this behaviour?

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Nov 5, 2015

I've thought the same thing...I wonder if it has to do with the actual math that's going on.

@hudochenkov
Copy link
Contributor Author

@hudochenkov hudochenkov commented Nov 5, 2015

Whatever math is, there is no problem to * -1 in code :)

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Nov 5, 2015

I agree. I thought about whether or not I might fix this a while back but then our old code would need some rework. I'd be curious to know why, like yourself, if there is something I'm missing.

@sdale-fevo
Copy link

@sdale-fevo sdale-fevo commented Jan 10, 2016

Hi,

I'm playing with Lost grid on Codepen. Struggled with this too.

My first attempt, with expected behaviors similar to other grid systems, Bootstrap, Materialize, Bourbon/Neat, I tried to offset to "push it" to the right, with a positive value of the number of columns I needed. For example, from Materialize

screen shot 2016-01-10 at 2 34 37 pm

And got nothing. Found this issue and tried negative, and got what I wanted.
Created a simple Codepen

The Offsetting Elements section in Readme could use a clearer update. Will try to submit a PR.

Might be worth an API change on next major update.
=: s

@corysimmons
Copy link
Contributor

@corysimmons corysimmons commented Jan 10, 2016

Yeah, sorry guys, this was actually a mistake that slipped in somewhere, or maybe I've just always thought of it backwards.

You guys all seem to agree that this is screwy so I definitely give the 👍 to convert this in 7 (super easy change).

@sdale-fevo
Copy link

@sdale-fevo sdale-fevo commented Jan 10, 2016

Great, thanks @corysimmons!

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Jan 11, 2016

Thanks for clarifying @corysimmons. I was curious about this myself. I'll update as an actionable issue.

@peterramsing peterramsing added this to the 7.0.0 milestone Jan 11, 2016
@peterramsing peterramsing changed the title lost-offset counterintuitive behavior Make Lost-Offset work more intuitively (reverse of current +/- behavior) Jan 11, 2016
@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Jan 25, 2016

To make this work, it's going to do some breaking.

7.0.0 is going to make lost-offset: 1/2 act like lost-offset: -1/2 acts now.
I'm thinking that there could be a global setting to revert this so that people could keep their code in production if they wanted to move to 7.0.0 yet still use the lost-offset syntax from sub Lost 7.0.

@corysimmons
Copy link
Contributor

@corysimmons corysimmons commented Jan 25, 2016

Perhaps instead of introducing a global setting specifically to cover up a mistake, we could make a script or something that would recursively traverse every file in a directory, regex for /(lost-offset: |-)/gm, then ternary to add a - or slice it off?

Then just add a link to the update script in 7.0.0 changelog.

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Jan 25, 2016

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Jan 25, 2016

@gwww 👍

@corysimmons
Copy link
Contributor

@corysimmons corysimmons commented Jan 25, 2016

I got started on it but couldn't figure out the whole readline write thing. Feel free to clone and fix. Gotta get some sleep.

https://github.com/corysimmons/lost-offset-upgrader.js/blob/master/lost-offset-upgrader.js

@hudochenkov
Copy link
Contributor Author

@hudochenkov hudochenkov commented Jan 25, 2016

@corysimmons what if someone doesn't put space between property and value lost-offset:1/2? It would be better to use PostCSS to parse CSS and make transforms.

And one more thing to consider: no script can guarantee 100% transform + to - and vice versa because user may use variables for value.

@corysimmons
Copy link
Contributor

@corysimmons corysimmons commented Jan 25, 2016

We can split/trim at colon so it wouldn't matter if they had lost-offset: 1/2; or lost-offset:1/2;.

For someone to get burned they would have to:

  • ignore the changelog on a major release (aka api breaking release)
  • store their offset values as variables
  • not read what the transform does

I think at that point I wouldn't feel too bad about it.

I agree with you on the whole node: property AST stuff with PostCSS, but how would implementing that look?

You'd make a PostCSS plugin the user had to configure into their gulpfile, run their gulpfile one time (and only one time), and then remove it from their gulpfile?

That script took 5 mins to make. I'm totally open minded to other solutions. My only concern was introducing new global variables to Lost and cluttering the API. :)

@gwww
Copy link

@gwww gwww commented Jan 25, 2016

While I'm generally against globals, in this case I am OK with it since the global variable is temporal. My preference is good changelog doc that says new behavior, work around for old behavior, and strict timeline for when deprecated behavior will disappear totally. My preference is because writing a script to "fix" old behavior is likely to be a source of much discussion in the form of "does it work in case ". Just my humble opinion as a newb here.

@corysimmons
Copy link
Contributor

@corysimmons corysimmons commented Jan 25, 2016

You're suggesting adding a temporary global variable that will get removed in 8.0.0 or something? That might work.

@gwww
Copy link

@gwww gwww commented Jan 25, 2016

Correct. Stays around until next major release or 6 months - whichever comes first. Perhaps:

settings.deprecated_behavior.offset = 1

Some projects, elixir for example, uses soft-deprecation then hard-deprecation. As I understand it soft keeps the feature around for months with changelogs and other mechanisms. Hard removes the feature.

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Jan 25, 2016

I am a fan of what @gwww said. Scripts are cool, and I think we could offer one, but I was thinking about a global setting that would allow for users to continue using Lost w/o having to do a large code-change.

@peterramsing peterramsing removed this from the 7.0.0 milestone May 23, 2016
@perkola
Copy link

@perkola perkola commented Aug 12, 2016

Any updates on this? I can't find any information in the release notes for the 7.0.* releases.

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Aug 12, 2016

@perkola I haven't put any time into this. I'm thinking this might wrap nicely into the Node updates/depreciation that's coming.

@peterramsing peterramsing modified the milestones: v7.1.0, v8.0 Aug 12, 2016
@motleydev
Copy link

@motleydev motleydev commented Sep 17, 2016

PLEASE add a byline about the negative offset in the existing docs. You made me question my very existence as a front-ender. :)

peterramsing added a commit that referenced this issue Sep 21, 2016
@corysimmons
Copy link
Contributor

@corysimmons corysimmons commented Sep 21, 2016

I'm sorry!!! 😫

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Sep 21, 2016

@corysimmons You're all good. 😄

@peterramsing peterramsing self-assigned this Oct 4, 2016
@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Oct 5, 2016

I have this working except now I'm seeing what might be considered a bug where the furthest right element in a row doesn't respond float left anymore because of #200 (PR: #218).

I'll figure out a way to make it so that the element that is floated right follows this... 🤔

...also related: #309

peterramsing added a commit that referenced this issue Oct 5, 2016
This reverses the current api from moving left to right based on
negative fractions which didn’t make much sense. This breaks that
api’s current functionality and makes it more intuitive
peterramsing added a commit that referenced this issue Oct 19, 2016
This reverses the current api from moving left to right based on
negative fractions which didn’t make much sense. This breaks that
api’s current functionality and makes it more intuitive
@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Oct 19, 2016

This is now in the Beta channel: http://peter.coffee/lostgrid-gets-beta-8-0-0

🎉

@hudochenkov
Copy link
Contributor Author

@hudochenkov hudochenkov commented Nov 1, 2016

Thank you!

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Nov 1, 2016

@hudochenkov Let me know if you have any issues with the beta!

@asifshirazi
Copy link

@asifshirazi asifshirazi commented Jan 18, 2017

@peterramsing Perhaps codepen has integrated with Lost v7.1.,
but after updating to v
8.0.0* on my local machine,
my append and prepend Lost helpers are not working properly.
http://codepen.io/shirazi/pen/PWbVyO

@peterramsing
Copy link
Owner

@peterramsing peterramsing commented Jan 19, 2017

@asifshirazi It looks like CodePen isn't up to date. I'll tweet them and see if it can get updated.

@corysimmons
Copy link
Contributor

@corysimmons corysimmons commented Jan 21, 2017

@peterramsing Coyier mentioned to me they were steering away from messing with PostCSS. Luckily, Jonathan Neal figured out a workaround: https://codepen.io/jonneal/post/testing-postcss-plugins

Now you can update without waiting or anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.