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

Adding rtl to support to 2.2.x/master branch #438

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@mattgoldspink

mattgoldspink commented Apr 4, 2017

For #308

Note: this is the same as an earlier PR I made but for 2.2.x. I understand this may not be merged, but again we need this for customers, so we have to do it anyway. I hope at somepoint we can see this merged so others may benefit and I'm happy to work with someone in Salesforce to get this done.

Changes proposed in this pull request:

  • Full RTL support by generating separate .rtl. stylesheets.
    • Adding directional-scss to flip the styles based on the preset $dir value (see https://github.com/tysonmatanich/directional-scss - MIT licensed)
    • New style sheets created for rtl versions of all stylesheets
    • Existing rules updated to use appropriate mixins, functions and variables.

NOTES
I have assumed that by default anyone using the "rtl" stylesheet will also have set <html dir="rtl"> on their page. This attribute gives the initial flip for most things (for example flexbox grids are automatically flipped for us).

I have gone through every component in the docs - even those we don't use - and fixed them. To do this I modified the scripts/gulp/pages.jsx to create the embedded example iframe with dir="rtl" and use the .rtl version of the stylesheet. I have NOT checked in this change, so if you want to manually see the rtl versions you'll need to reproduce this:

image

I have left checkbox ticks facing the ltr way (I'm not an expert on rtl languages but what I've seen in the past is a that things like ticks don't change direction just because it's rtl). However the checkbox is position on the alternate side of existing elements.

I did not flip some arrows as I'm not sure if we should create additional CSS rule to use transform on these or use the opposite SVG arrow direction (for example on Tree expanding).

Salesforce CLA will be signed shortly.

Reviewer, please refer to this "definition of done" checklist:

  • Tested on desktop (see supported browsers)
    In Chrome 56 , Firefox 52 (on Mac), IE11 Win7
  • Tested on mobile (for responsive or mobile-specific features) - No, which devices should be tested for this?
  • Documentation is up to date - Not yet, I'm happy to do this, but just need some direction as to where this should end up
  • Release notes mention the changes - same as above.

Some sample screenshots:

Activity Timeline
image

Data Tables
image

Process
image

@salesforce-ux-bot

This comment has been minimized.

Collaborator

salesforce-ux-bot commented Apr 4, 2017

@mattgoldspink, thanks for your PR! By analyzing the history of the files in this pull request, we identified @brandonferrua, @stefsullrew and @aputinski to be potential reviewers.

@@ -62,7 +62,7 @@
}
&.slds-media--small .slds-media__figure {
margin-left: $spacing-xx-small;
margin-#{$left}:$spacing-xx-small;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

}
.slds-media--large .slds-media__figure--reverse {
margin-left: $spacing-large;
margin-#{$left}:$spacing-large;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

}
.slds-media--small .slds-media__figure--reverse {
margin-left: $spacing-xx-small;
margin-#{$left}:$spacing-xx-small;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

}
.slds-media--large .slds-media__figure {
margin-right: $spacing-large;
margin-#{$right}:$spacing-large;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

@@ -30,19 +30,19 @@
}
.slds-media--small .slds-media__figure {
margin-right: $spacing-xx-small;
margin-#{$right}:$spacing-xx-small;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

@@ -11,7 +11,7 @@
.slds-item--label {
width: 30%;
padding-right: $spacing-small;
padding-#{$right}:$spacing-small;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

}
&--right {
margin-left: auto;
margin-#{$left}:auto;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

}
}
&--left {
margin-right: auto;
margin-#{$right}:auto;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

left: auto;
right: auto;
#{$left}:auto;
#{$right}:auto;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

@@ -33,16 +33,16 @@
&--center {
margin: {
left: auto;
right: auto;
#{$left}:auto;

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Colon after property should be followed by one space or a newline

@kaelig

This comment has been minimized.

Contributor

kaelig commented Apr 7, 2017

Closing/reopening this to see if the CLA bot gets this.

@kaelig kaelig closed this Apr 7, 2017

@kaelig kaelig reopened this Apr 7, 2017

@mattgoldspink

This comment has been minimized.

mattgoldspink commented Apr 7, 2017

I just signed the cla so hopefully that should fix the error there.

@kaelig

This comment has been minimized.

Contributor

kaelig commented Apr 7, 2017

Thank you @mattgoldspink! Looks like we're having some issues with the CLA bot, apologies. We're on it!

}
$left: if-ltr(left, right);
$right: if-ltr(right, left);

This comment has been minimized.

@3den

3den Apr 7, 2017

It is confusing that right can mean left and left can mean right, the value of right and left should be absolute to avoid confusion. To be rtl friendly the names should be generalized, like start and end.

<div class="slds-rtl"> 
    <p class="slds-text-align--left">I should stay on the left no matter what.</p>
    <p class="slds-text-align--start">
        I should go right when a parent has slds-rtl, or left otherwise.
    </p>

      <p class="slds-text-align--end">
         I should go left when a parent has slds-rtl, or left otherwise.
      </p>
 
      <div class="slds-ltr">
         <p class="slds-text-align--start">
             I am going left coz my closest parent has `slds-ltr`
         </p>
      </div>
</div>

This comment has been minimized.

@mattgoldspink

mattgoldspink Apr 7, 2017

Thanks for the feedback - In all honesty I'm not an rtl pro. I based this patch of the following article: http://matanich.com/2013/09/06/rtl-css-with-sass from which the above file was taken.

I'm trying to get my head around how what you're proposing in general would work for the components when it comes to things like margins, paddings, etc. For text I can see that a slds-text-align--start would work, but what about flipping the layout of say the docked-composer: https://github.com/salesforce-ux/design-system/pull/438/files/8d3423da2129117c8cc5cd383966b18efa231763#diff-0258d2cd8c34aac1a2fac1c7a226e2e7

Would it be better to provide an override rule such that if the parent element has [dir=rtl] then we flip padding/margins? That would keep things in one css file too.

This comment has been minimized.

@mattgoldspink

mattgoldspink Apr 8, 2017

I've been reworking the code so that left stays left. As I mentioned about, where this gets tricky is when a component (e.g. activity timeline) uses slds-m-left--large. Given what's been said about text alignment and left always being left, should we also apply the same logic for padding/margins? I.e. people would need to change their DOM to use slds-m-start--large instead and therefore get access to rtl features?

The big downside I see to this is that it means the docs have been pushing people in a non-rtl compatible direction and everyone would need to change their dom to get rtl for free. Whereas if we do allow slds-m-left--large to actually be margin-right when embedded in a [dir=rtl] parent means that all components can become rtl compatible out of the box. So when Lightning Experience finally does support RTL, people's custom Lightning components won't automatically be rtl compatible. Maybe that's a good thing? Because chances are most people don't initially write them with rtl support in mind.

What do others think about this? Should left stay meaning left and we introduce start/end versions of everything that people migrate to for rtl support?

This comment has been minimized.

@mattgoldspink

mattgoldspink Apr 10, 2017

A few more challenges to the approach of keeping left as left in all scenario's. Take the form input where someone adds [dir=rtl] but keeps the slds-input-has-icon--left:

<div class="slds-form-element">
  <label class="slds-form-element__label" for="text-input-01">Input Label</label>
  <div class="slds-form-element__control slds-input-has-icon slds-input-has-icon--left">
    <svg class="slds-input__icon slds-icon-text-default" aria-hidden="true">
      <use xlink:href="/assets/icons/utility-sprite/svg/symbols.svg#search"></use>
    </svg>
    <input type="text" id="text-input-01" class="slds-input" placeholder="Placeholder Text" />
  </div>
</div>

The icon will stay on the left, but by default the text will be sent to the right:

image

Unless you then add direction: ltr explicitly to it- which is of course an option. But I think the biggest problem comes from having to reset padding's, etc when using start, end. You end up with rules like this:

&--left,
  &--start {
    .slds-input__icon {
      left: ($spacing-medium + $spacing-xx-small);
    }

    .slds-input,
    .slds-input--bare {
      padding-left: $spacing-xx-large;
    }
  }

[dir=rtl] {
    &--start {
      .slds-input__icon {
        right: ($spacing-medium + $spacing-xx-small);
        left: 0;
      }

      .slds-input,
      .slds-input--bare {
        padding-right: $spacing-xx-large;
        padding-left: 0;
      }
    }
}

Aside from it being ugly, it becomes a problem when someone has added slds-p-left--large as well - the new rule would take precedence due to it's specificity being higher.

This comment has been minimized.

@3den

3den Apr 11, 2017

hi @mattgoldspink you are bring many valid points, the switch you did initially would be least disruptive to SLDS current version, but I imagine that even from rtl languages, building an app that is rtl only, it would feel confusing to type -left when they want right and -right when they want left... From what it seems SLDS was not build to be non-rtl compatible.

Take a look at Atomic (https://acss.io/guides/syntax.html) that was built with rtl support in mind since the beginning.

This comment has been minimized.

@3den

3den Apr 11, 2017

The last code sample you sent for the .slds-input is very good, the problem of adding slds-p-left--large and having overlaps is the same currently without the rtl support.

I think the directional stuff can be separated from the regular code, maybe on flavors/directional/_index.scss, so on the current code we just need to add -start and -end where we use -left and -right maybe -left and -right can be deprecated in future versions. An on the flavors/directional/_index.scss we can add only the rtl overrides.

[dir=rtl] {
   .slds-table--edit_container {
      .slds-table--edit_container-message {
          right: 50%;
          margin-right: (($size-medium / 2) * -1);
      }
   }
}

This comment has been minimized.

@mattgoldspink

mattgoldspink Apr 12, 2017

Thanks again for the feedback. I've started on something that roughly maps to what you suggested above. I have it done for activity timeline, buttons, forms and few other components now. I'll keep working through the rest and then test against our rtl orgs before resubmitting the PR. I've also been updating the docs too to include start and end examples where it makes sense

@@ -62,7 +62,7 @@
}
&.slds-media--small .slds-media__figure {
margin-left: $spacing-xx-small;
margin-#{$left}: $spacing-xx-small;

This comment has been minimized.

@3den

3den Apr 7, 2017

This will make impossible to share the same css on a site that supports both.

A common use case is to have a site that is ltr by default but can be toggled to rtl for users on a specific region or based on a config.

<body className={isRtl ? 'slds-rtl' : 'slds-ltr'}>
   ... `-end` class modifier with mean `left` only if the parent is 'slds-rtl' 
   ... `-start` class modifier with mean `right` only if the parent is 'slds-rtl'
</body>

This comment has been minimized.

@mattgoldspink

mattgoldspink Apr 7, 2017

Our use case is a client based in Israel so for us loading a different css is fine, but I understand there's other use cases to cater for others, so I'd rather get something that everyone can use (including us) and we're happy to do this for the community.

Is it better to detect rtl off the [dir=rtl] attribute on a parent element instead of adding an slds-rtl class?

This comment has been minimized.

@3den

3den Apr 11, 2017

You are right, using the [dir=rtl] is much better than adding a class.

This comment has been minimized.

@stefsullrew

stefsullrew Apr 11, 2017

Member

That's how I did it when I did rtl for kanban (many, many moons ago). I built everything off the [dir=...

@mattgoldspink

This comment has been minimized.

mattgoldspink commented Apr 8, 2017

@3den Thanks for the feedback - let me work on a different approach for this patch which doesn't use _directional.scss and require such extreme changes to all the files.

I think what you said about requiring a seperate CSS file for rtl when a page may contain mixed ltr and rtl text is a pretty big limitation. So I'll see if I can rework this to better handle that case and include some of your other suggestions

@ishakasliwal

This comment has been minimized.

Contributor

ishakasliwal commented Apr 12, 2017

Thanks so much for all of your hard work with this contribution, @mattgoldspink ! We are happy to see that you've found a meaningful solution that can work for your customers.

Since we follow a staggered release schedule internally, and because this work on rtl has unknown risks and implementation details, we unfortunately will not be able to merge this pull request. However, your work has greatly benefited us, and when we get a chance to return to rtl internally, we will look to your solution for inspiration on how to solve the problem.

If you'd still like feedback from us, we are happy to continue the conversation over Github issues. Thanks again!

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