feat(Text): add align prop

[bmdalex]

feat(Text): add textAlign prop

BREAKING CHANGE MITIGATION

Header's textAlign prop values changed from 'left' | 'center' | 'right' | 'justified' to 'start' | 'end' | 'center' | 'justified'

mitigation: change textAlign="left" to textAlign="start" and textAlign="right" to textAlign="end"

Description

This PR adds textAlign prop to the Text component in order to allow users to align content to the left, right, center or have it justified.

[mnajdova]

Probably you already considered this, but why don't we just add display: 'block' in the styles if the textAlign prop is provided?

[bmdalex]

Good that you opened this conversation. I have considered it briefly but thought it might be unexpected behavior for the user.

Giving it a second thought if a user adds textAlign they probably expect the Text component to behave as a block otherwise alignment does not make sense. So I think you're right here. Let me make the change and ask others to chip in as well: @layershifter @kuzhelov @miroslavstastny

[bmdalex]

applied display: 'block' when textAlign prop is provided

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@0a4cab0). Click here to learn what that means.
The diff coverage is 8.69%.

@@            Coverage Diff            @@
##             master    #1668   +/-   ##
=========================================
  Coverage          ?   70.97%           
=========================================
  Files             ?      860           
  Lines             ?     7142           
  Branches          ?     2052           
=========================================
  Hits              ?     5069           
  Misses            ?     2067           
  Partials          ?        6

Impacted Files	Coverage Δ
packages/react/src/components/Header/Header.tsx	100% <ø> (ø)
...src/themes/teams/components/Header/headerStyles.ts	20% <ø> (ø)
packages/react/src/components/Text/Text.tsx	100% <ø> (ø)
...eact/src/themes/base/components/Text/textStyles.ts	0% <0%> (ø)
...act/src/themes/teams/components/Text/textStyles.ts	0% <0%> (ø)
packages/react-proptypes/src/index.ts	34.72% <100%> (ø)
packages/react/src/styles/translateAlignProp.ts	12.5% <12.5%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a4cab0...2b936ed. Read the comment docs.

[mnajdova]

LGTM, nice cleanups too! 👍

[miroslavstastny]

My main question is whether we want to have a design term for alignment (like we had here previously) or expose a CSS property like you are doing in this PR.

My opinion is that we should stick with a design term, for that reason left, center, right, justified work for me better that all the valid CSS values (-moz-initial is definitely not a design term, if you need it, use styles).
And I would even think about replacing left and right with start and end to comply with our direction-agnostic terminology.

[mnajdova]

I agree with @miroslavstastny points. I guess @Bugaa92 was just following the way it is already implemented in the Header component. Maybe the best option would be to introduce a different prop (something that does not necessarily correlate directly to css prop), and provide the mention values there as options.

[bmdalex]

@miroslavstastny - very good points, I'm confident others agree; I see @mnajdova liked the message so 3 votes already.

The only issue I have with our start and end as values of textAlign prop is that they mean something different for the actual textAlign CSS prop.

So when we'll have

<Text textAlign="end" .. />

will we render

<span style="text-align:right" ../>

or

<span style="text-align:end" ../>

?

These have different functionality in RTL mode as you can see here:
https://codesandbox.io/s/scrollableelement-wm5tb

The options I see right now are:

1. have textAlign prop with values 'start' | 'end' | 'center' | 'justified' and forward values as they are to CSS textAlign

2. have textAlign prop with values 'start' | 'end' | 'center' | 'justified' and:

• forward values as they are to CSS textAlign for center and justified
• convert start and end prop values to left and right CSS values

3. keep textAlign prop with values 'left' | 'right' | 'center' | 'justified' (as it is now for Header) and forward values as they are to CSS textAlign

4. have textAlign prop with values 'before' | 'after' | 'center' | 'justified' and:

• forward values as they are to CSS textAlign for center and justified
• convert before and after prop values to left and right CSS values

@mnajdova @miroslavstastny what's your take on all this?

[miroslavstastny]

FWIW in your codesandbox example you are not using Stardust Provider, with that the results are different in RTL:

Source

  <>
  LTR Provider
  <Provider styles={{border: '2px dashed red', padding: '20px'}}>
      <Text textAlign="right">Test align right</Text>
      <Text textAlign="end">Test align end</Text>
  </Provider>
  <br/>
  RTL Provider
  <Provider rtl styles={{border: '2px dashed red', padding: '20px'}}>
      <Text textAlign="right">Test align right</Text>
      <Text textAlign="end">Test align end</Text>
  </Provider>
  </>

So this makes end CSS value useless in my eyes.

I would go with option 2 and, as @mnajdova suggests, think about renaming textAlign prop to mentally detach it from the CSS prop.

[bmdalex]

I would go with option 2

done

as @mnajdova suggests, think about renaming textAlign prop

what about having it just align? we already have it for other components (Menu, Dropdown, FlexItem) and even though it has different meaning in the other component I think it's obvious what the meaning is for Text and Header.

So, what do you guys think about renaming textAlign prop to align? @mnajdova @miroslavstastny

[mnajdova]

Agree, I vote for align too

[bmdalex]

done, thanks guys!

[mnajdova]

I think justified doesn't work for the Header component:

Actually the same is for the Text component too. The reason is that the textAlign prop expect: justify, not justified. We should either translate this correctly, or change the typings...