Calcite-components: Bug: Remove setting `dir=` on components

[driskull]

Summary

Bug: Remove setting dir= on the host of components

Actual Behavior

Components are setting the dir attribute on their host and child components. When they do so, it makes it difficult to switch direction in an application because the author has to remove all of these dir attributes everywhere.

Expected Behavior

Instead, we should be using the utility class or only setting dir on shadowed elements.

https://github.com/Esri/calcite-components/blob/master/src/utils/resources.ts#L2

Relevant Info

62 results - 40 files

• [x] src/components/calcite-accordion-item/calcite-accordion-item.tsx: @driskull
• [x] src/components/calcite-alert/calcite-alert.tsx:
• [x] src/components/calcite-avatar/calcite-avatar.tsx: @driskull
• [x] src/components/calcite-button/calcite-button.tsx: @driskull
• [x] src/components/calcite-card/calcite-card.tsx: @driskull
• [x] src/components/calcite-checkbox/calcite-checkbox.tsx: @driskull
• [x] src/components/calcite-chip/calcite-chip.tsx:
• [x] src/components/calcite-color-picker/calcite-color-picker.tsx: Not needed.
• [x] src/components/calcite-color-picker-hex-input/calcite-color-picker-hex-input.tsx: Not needed.
• [x] src/components/calcite-combobox/calcite-combobox.tsx: @driskull
• [x] src/components/calcite-combobox-item/calcite-combobox-item.tsx:
• [x] src/components/calcite-combobox-item-group/calcite-combobox-item-group.tsx: @driskull
• [x] src/components/calcite-date-picker/calcite-date-picker.tsx:
• [x] src/components/calcite-date-picker-day/calcite-date-picker-day.tsx:
• [x] src/components/calcite-date-picker-month/calcite-date-picker-month.tsx: Not needed.
• [x] src/components/calcite-date-picker-month-header/calcite-date-picker-month-header.tsx:
• [x] src/components/calcite-dropdown/calcite-dropdown.tsx: @driskull
• [x] src/components/calcite-dropdown-group/calcite-dropdown-group.tsx: @driskull
• [x] src/components/calcite-dropdown-item/calcite-dropdown-item.tsx:
• [x] src/components/calcite-fab/calcite-fab.tsx: Not needed.
• [x] src/components/calcite-input/calcite-input.tsx:
• [x] src/components/calcite-input-date-picker/calcite-input-date-picker.tsx: @driskull
• [x] src/components/calcite-input-message/calcite-input-message.tsx: @driskull
• [x] src/components/calcite-label/calcite-label.tsx: @driskull
• [x] src/components/calcite-link/calcite-link.tsx: @driskull
• [x] src/components/calcite-modal/calcite-modal.tsx:
• [x] src/components/calcite-notice/calcite-notice.tsx:
• [x] src/components/calcite-pagination/calcite-pagination.tsx:
• [x] src/components/calcite-pick-list/shared-list-render.tsx: Not needed.
• [x] src/components/calcite-radio-button/calcite-radio-button.tsx: Not needed.
• [x] src/components/calcite-radio-group-item/calcite-radio-group-item.tsx: Not needed.
• [x] src/components/calcite-rating/calcite-rating.tsx:
• [x] src/components/calcite-select/calcite-select.tsx:
• [x] src/components/calcite-split-button/calcite-split-button.tsx:
• [x] src/components/calcite-stepper/calcite-stepper.tsx:
• [x] src/components/calcite-stepper-item/calcite-stepper-item.tsx:
• [x] src/components/calcite-switch/calcite-switch.tsx: Not needed.
• [x] src/components/calcite-tab-title/calcite-tab-title.tsx:
• [x] src/components/calcite-tile-select/calcite-tile-select.tsx:
• [x] src/components/functional/CalciteExpandToggle.tsx: Not needed.

Example

import { getElementDir } from "../../utils/dom";
import { CSS_UTILITY } from "../../utils/resources";

class={{ [CSS_UTILITY.rtl]: dir === "rtl" }}

.calcite--rtl .toggle--switch calcite-switch {
  margin-left: 0;
  margin-right: var(--calcite-spacing-half);
}

[caripizza]

I'm going to take a stab at calcite-chip to start

[tjameswhite]

I'm not sure this approach is going to solve the problem -- it looks like this just moves it up one level. If I'm reading this right you have moved component level dir to a class wrapping the element.

We are setting dir on the body. This needs a pattern more like [dir="rtl"] [calcite-component] {//do something}

[driskull]

If I'm reading this right you have moved component level dir to a class wrapping the element.

Actually, it moves it down a level. It keeps the class internal.

This still doesn't solve telling all the components to re-render if a higher level dir attribute changes. We would need some sort of store for that.

[caripizza]

@driskull should this file be updated too?
https://github.com/Esri/calcite-components/blob/master/conventions/README.md#i18n

To add RTL support to your components you should use the internal getElementDir helper to apply the dir attribute to your component. This means that your components dir attribute will always match the documents dir.

[driskull]

Yes definately

[paulcpederson]

@driskull yeah adding a store would be awesome. Going to write some thoughts. Maybe we should make a new issue to capture this?

In the component I think it would look like:

import { calciteComponentsStore as store } from "../../utils/store";

const dir = store.dir || getElementDir(el);

The store itself would be pretty simple:

import { createStore, ObservableMap } from "@stencil/store";

export interface CalciteComponentsState {
  dir?: "rtl" | "ltr"
}

export type CalciteComponentsStore = ObservableMap<CalciteComponentsState>;

export const calciteComponentsStore = createStore<CalciteComponentsState>({});

Not totally sure how you would get the store into the library to allow end devs to manipulate it. Something like this would be great, but I don't know if it's possible:

import { defineCustomElements, calciteComponentsStore } from "@esri/calcite-compomnents/dist/loader";

calciteComponentsStore.dir = "rtl";
defineCustomElements(window);

[driskull]

Yeah I don't think we should tackle the store at this point. I think this issue is a step in the right direction though so we're not internally setting dir which then becomes difficult to change.

[driskull]

I took a stab at some of them. Could others put their GitHub handle next to the remaining components?

[paulcpederson]

step in the right direction

I see what you did there.

[ethanbdev]

Quick question @driskull, does this change make it so the dir attribute does not need to be set on the component for it to pick up the language direction?

[driskull]

The dir would need to be set on the component or on of its ancestors. It doesn't change that.

It just makes it so we're not automatically setting it ourselves

[ethanbdev]

The dir would need to be set on the component or on of its ancestors. It doesn't change that.

It just makes it so we're not automatically setting it ourselves

Cool, thanks 😃

[driskull]

Updated issue to reflect that we can still set the dir attribute on shadowed elements. Just not on light DOM elements or on the host.

Eventually, we can re-render shadowed DOM elements when necessary based on an ancestor dir change.

[driskull]

@julio8a @paulcpederson @macandcheese

I can't complete the following components. They need refactoring to the host styling in order to remove the 'dir' attribute. Since these components set things like padding or margin on the host element and then modify them when in RTL, they need those styles moved to be internal to the component so that a selector doesn't have to be applied to the host in light dom.

For example, there is conditional padding applied to the host element here:

// RTL
:host([dir="rtl"][scale="s"]) {
  @apply pr-6 pl-2;
}

• calcite-dropdown-item
• calcite-notice
• calcite-stepper-item

We can fix this by moving the styles into the shadow dom of the component, or by using CSS statements that automatically change in RTL.

[driskull]

@jcfranco @paulcpederson @julio8a should we have an issue to start using flow-relative css values like start/end and when we can start doing that conversion? This would get rid of 99% of the use for getElementDir()

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Logical_Properties

[paulcpederson]

Yeah, I wish tailwind used those properties instead, honestly. Like it would be great if mr-1 just became margin-inline-end: .25rem. Not sure if that's possible...

[driskull]

@paulcpederson could we use this? https://github.com/20lives/tailwindcss-rtl

[driskull]

Or this one: https://github.com/stevecochrane/tailwindcss-logical

[macandcheese]

Found this as well - https://github.com/stevecochrane/tailwindcss-logical#flow-relative-margins - seems like we could also just add our own custom set without a plug-in too for margin and padding at least.

[driskull]

Yeah maybe we can do this in this order:

• Finish host dir removal
• Finish tailwind refactor
• tailwind v2 update
• tailwindcss logical and remove dir usage.

[paulcpederson]

@macandcheese I think we'd want the border-right/left, border-radius, and right/left sets as well, so a plugin is probably better. I don't think we support the vertical text mode, so maybe we could remove some of those or customize it.

@driskull that sounds like the right order. We could do a logical property thing before a v2 upgrade but we'd have to make sure whatever plugin was supported in 2 as well. Theoretically we should be able to just change the tailwind config/plugins and not have to change any code for the logical properties upgrade.

[julio8a]

Looping in @caripizza in this conversation as it involved refactor work.
https://github.com/Esri/calcite-components/issues/1831#issuecomment-828799982

[driskull]

Installed. needs verification.

[caripizza]

~To-do: add an issue to create an eslint rule (check dir attribute not applied) along with a test - in order to fully verify this one (for 5/10-5/21 sprint)~ (done)

[caripizza]

@driskull @jcfranco do you think all components should have RTL stories for Storybook? Meaning, the story html snippet would have dir="rtl" (not just a knob). Would that help with verification on this one so we can have the new standard checked via Screener moving forward? (If so I'll create the issue)

[driskull]

Yes, that sounds good for storybook and testing.

[jcfranco]

Verified locally using custom `@calcite-components/ban-props-on-host' ESLint rule. The RTL Screener work will be tackled by https://github.com/Esri/calcite-components/issues/2108.