FirstLegoLeague / displaySystem

Set of display tools
https://firstlegoleague.github.io/displaySystem/
4 stars 7 forks source link

rtl module #23

Closed 2roy999 closed 8 years ago

2roy999 commented 8 years ago

Adding module to support RTL languages

rikkertkoppes commented 8 years ago

Thanks for that, this is definitely a useful feature. I'll have a closer look in the weekend.

Could you elaborate a bit on the pros en cons of solving this with css alone? In other words, why do you need a module?

2roy999 commented 8 years ago

well I started it as only a css theme, but I made a module because I needed to override some of the other modules styles. Anyway while writing this mail I remembered the !importent feature of css that was missing to me while writing the css.

I will add a new pull request with only the new theme.

On Thu, Jun 23, 2016 at 9:46 PM, Rikkert Koppes notifications@github.com wrote:

Thanks for that, this is definitely a useful feature. I'll have a closer look in the weekend.

Could you elaborate a bit on the pros en cons of solving this with css alone? In other words, why do you need a module?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/FirstLegoLeague/displaySystem/pull/23#issuecomment-228146527, or mute the thread https://github.com/notifications/unsubscribe/ACxO-RsWF4Xwof35IdH7HEUJMX6FHhV9ks5qOtR8gaJpZM4I9HQb .

להתראות, רועי שמואלי

rikkertkoppes commented 8 years ago

Well, I haven't actually made up my mind about it yet. There are a few options

Leaning towards the latter, but not sure. I'll ask others to chime in

2roy999 commented 8 years ago

Well the only problem I see with css write now is the twitter which I didn't touch yet. The thing is that css theme seems to me much more clean then the module.

also the fact that I used a module only let me adding class rtl to the body, I still edited every module style to support the RTL. So I think the css is better solution.

On Thu, Jun 23, 2016 at 11:19 PM, Rikkert Koppes notifications@github.com wrote:

Well, I haven't actually made up my mind about it yet. There are a few options

  • make it a module, modifying other modules behaviour (for the twitter ticker, we may want to scroll from left to right for rtl mode)
  • make it a theme, overriding module css. As do all themes
  • make it a core feature, let modules define for themselves how to behave.

Leaning towards the latter, but not sure. I'll ask others to chime in

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/FirstLegoLeague/displaySystem/pull/23#issuecomment-228171104, or mute the thread https://github.com/notifications/unsubscribe/ACxO-S6DDDX9DNA3FuAlRePZQ7j0Zkuxks5qOupSgaJpZM4I9HQb .

להתראות, רועי שמואלי

poelstra commented 8 years ago

First of all: nice work @2roy999!

Like @rikkertkoppes says, there are basically 3 options to fix this. I suppose RTL is common enough that it would be good to have it as a core feature.

I wonder how much needs to change to most modules to 'properly' support RTL. For example, if (for most modules) it's indeed sufficient to basically apply a "direction: rtl" from some base style and be done with it, it might work to just put it in core.

However, I already saw that you e.g. had to mirror images as well (maybe that can also be done generically using styles, too). If other things also need to change (default positions of elements, e.g. from top right to top left corner) etc etc, then maybe people would have to 'actively' know how to support RTL in their modules to make it work properly.

In that last case, I suppose that 'non-RTL-aware' modules will typically have some issues, which will probably still make the 'override mode' approach of #23 and #24 necessary in some cases.

I suppose we'll end up doing a bit of both: make it a core feature such that most modules will automatically work (e.g. clock module), and that more complex modules can natively support it (e.g. twitter feed), but that we still allow people to apply their own stylesheet on top of all existing ones, to e.g. fix issues with modules that don't work correctly with RTL yet.

This way, you could still do stuff like #24, but it would be only necessary for 'problematic' modules as a temporary workaround until the module has built in RTL support.

Not sure what it would take to put it in core, though. @rikkertkoppes likely has good ideas here.

rikkertkoppes commented 8 years ago

Close in favor of #24