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

Add option to enable both RTL and LTR styles in one stylesheet #360

Open
chaenu opened this issue Mar 6, 2017 · 8 comments
Open

Add option to enable both RTL and LTR styles in one stylesheet #360

chaenu opened this issue Mar 6, 2017 · 8 comments

Comments

@chaenu
Copy link

chaenu commented Mar 6, 2017

Hey,
first of all thank you for you work, I used lost grid in several projects and it worked great!

Is this a feature request or a bug report?
Feature request.

What is the current behavior?
With the current lost grid version (8), it's only possible to use either ltr or rtl styles. To mix them in the same project, you need to define two stylesheets and prefix the related classes "manually" with e.g. [dir="rtl"] (see below).

What is the behavior that you expect?
Be able to use rtl and ltr in the same stylesheet.

What's the motivation or use-case behind changing the behavior?
I am currently working on a global website, including left-to-right and right-to-left pages which should both rely on the same codebase/stylesheet.

Anything else?
I could think of a new value for the direction global option, e.g.
@lost --beta-direction both;
which would render both ltr and rtl styles, but adding the dir attribute as a selector, e.g.
[dir="rtl"] .my-grid-col:nth-child(1n) { margin-left: 40px; margin-right: 0; }
[dir="ltr"] .my-grid-col:nth-child(1n) { margin-left: 0; margin-right: 40px; }

Related: #351 & #352

What do you think?

@peterramsing
Copy link
Owner

peterramsing commented Mar 7, 2017

@chaenu I like it.

I'm still trying to sort some of the "how does pushing and pulling elements work if their order is reversed...what is 'right', what is 'left'?"

Are you up for putting together a PR for this?

@chaenu
Copy link
Author

chaenu commented Mar 7, 2017

Yes, I will try! Any rough idea how to implement this best?

@peterramsing
Copy link
Owner

peterramsing commented Mar 8, 2017

@chaenu Super. If you want to create a PR early-on we can coordinate the changes.

Here's the initial PR: https://github.com/peterramsing/lost/pull/352/files
That should get you some general understanding of what went into the change.

I'm not sure the implementation of making it dynamic based on how you described but I think that it's certainly something that could be put on the initial line such as

lost-column: 1/3 rtl;

...or something like that.

See in this area where/how you might do that: https://github.com/peterramsing/lost/blob/master/lib/lost-column.js#L56

I will say, there is a lot of potential for cleanup and optimization here. One thing that will be tough is that this RTL support can easily cause the rest of the code-base to become a bit hard to maintain...and while I really do want to get some awesome RTL support in LostGrid I don't want it to cause other features to be hindered from being added or existing features to be harder to maintain.

Thanks again and let me know how I can help!

@peterramsing
Copy link
Owner

@chaenu If you want to pair on this let me know. Skype? Screenhero?

@chaenu
Copy link
Author

chaenu commented Mar 11, 2017

Hey Peter,

you can see the current result here: master...chaenu:feature/rtl
What do you think so far? I followed your suggestion with rtl as an argument for lost-column and lost-waffle, so the developer itself has still full control over the final selector.

I also think there's a bug in the line:
lostColumnCycle = decl.value.split('/')[1];
E.g. if you add an argument, then lostColumnCycle contains not only the number of the cycle, but also all arguments. Tried to fix that as well.

Current tests are ok so far, but I will check tomorrow if I should add more tests (and documentation, of course).

@peterramsing
Copy link
Owner

That's looking like a great direction! I'll create a PR into develop and let's see what tests we'll need to write and then look into 1. making sure the existing grids don't break and 2. making sure it's not muddying things up too much. The RTL support might warrant re-thinking the core lost-column/lost-waffle architecture – but thankfully RTL is in beta so we have the freedom to change as we need.

@peterramsing peterramsing mentioned this issue Mar 11, 2017
2 tasks
@chaenu
Copy link
Author

chaenu commented Mar 12, 2017

Thank you for your feedback! I added tests (I'm not that experienced writing them, so I hope I covered all relevant cases) and documented the option.

I just struggled over the float-right option for lost-waffle. If I understand the use case correctly, then we would need to introduce a float-left option for rtl-layouts, right? Or maybe update this to a more general feature "last-line-different-direction", no matter if ltr or rtl is used (breaking change...).

Yes, re-thinking the core lost-column / lost-waffle architecture could be a good idea, as they share mostly the same features and adding rtl to them feels mostly like duplicating code. :)

@peterramsing
Copy link
Owner

...not only like duplicating code but it's only designed to go one direction so the code starts to get really confusing if the grid direction is right-to-left. (not my FIXME comment)

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

No branches or pull requests

2 participants