FIX - Revise LTR/RTL configuration to not use import + if statements #263
Conversation
| @if $layout-direction == rtl { | ||
| $grid-direction-default: rtl; | ||
| $grid-direction-reversed: ltr; | ||
|
|
|
🌟 LGTM 🌟 |
| // ------------------------------ | ||
| // #LIB | ||
| // ------------------------------ | ||
| @import '../../../node_modules/bi-app-sass/bi-app/bi-app-ltr'; |
There was a problem hiding this comment.
Nit: I'm wondering if we should add partials for _ltr.scss and _rtl.scss. I'm not super comfortable with every client of the pattern library having to know that we're using bi-app, and it would make it harder to change that dependency. I'm also wondering if we might eventually have some additional actions that we'd perform in these partials, so having them broken out now would make that easier.
Does that make sense? Am I missing something as to why this is a bad idea?
There was a problem hiding this comment.
In fact, that partial could set the $layout-direction too so that wouldn't have to be repeated.
There was a problem hiding this comment.
Thanks, @andy-armstrong! That's damn good feedback and simplifying this more makes sense.
I've gone ahead and done that with this fixup! commit (a4dc34d). The README file compilation examples have also bee updated. How does this sound?
There was a problem hiding this comment.
Looks great. Thanks!
|
👍 Looks great to me. |
talbs
force-pushed
the
talbs/fix/ltr+rtl-configuration
branch
from
462cb7f
to
a4dc34d
Compare
|
@andy-armstrong and @dsjen, I addressed your good feedback. Let me know if you want to take another spin through or if I should move ahead with merging this and using it on our next steps in the platform work. |
| ``` | ||
|
|
||
| **NOTE**: Since both libSass and RubySass lack a way to pass in variables/configuration into their ``@import {file}`` method, each app is responsible for 1) storing any npm-based dependencies, including the edx-pattern-library, in the best directory structure for that app's set up and 2) creating a ``_lib.scss`` partial to import all third party library dependencies from that structure for the Pattern Library to use (see above example). | ||
|
|
||
| **NOTE**: We support right-to-left and left-to-right-based layouts. To suppor this within each app, we recommend including a ``_rtl.scss`` or ``_ltr.scss`` file that contains the right layout setting and reference to specific third party utilities. |
There was a problem hiding this comment.
When you say "we recommend", does this mean that you can choose not to include one or the other? Does it matter that $layout-direction won't be set?
|
👍 |
talbs
force-pushed
the
talbs/fix/ltr+rtl-configuration
branch
from
f152346
to
322cc26
Compare
FIX - Revise LTR/RTL configuration to not use import + if statements
We ran into the issue of edx-platform Sass not being able to compile UXPL-based project Sass compilation files due to the following error:
After looking into things more via the following threads, its best to move away from leveraging importing RTL/LTR support using a Sass
@ifclause.This work revises the LTR/RTL config and settings to not rely on Sass
@import+@ifsince Sass doesn't support@importstatements inside of@ifsyntax. More details on the steps include:_grid.scssReviewers
If you've been tagged for review, please check your corresponding box once you've given the 👍.
Post-review