feat(text): Add `as` prop while deprecating `elementType` #631

jonnybel · 2019-04-03T13:19:04Z

Summary

This PR adds a as prop to the Text components, as suggested by @tdeekens in #588 (comment).

Deprecates elementType prop of Text.Headline and Text.Subheadline components.
Deprecates isInline prop of Text.Body component.

Questions

Which element types should we allow to render for the Body component? (currently just span and p)

emmenko · 2019-04-03T13:24:09Z

src/components/typography/text/text.js

+            "${propName}" property of "${componentName}" has been deprecated
+            and will be removed in the next major version.\n
+            Please use "as" prop instead.`;
+      warning(false, message);


This will cause a massive amount of warnings.

I would proceed as following instead:

introduce the as prop without touching the other props

wait for a new major bump

deprecate the elementType prop and add the warning

wait for a new major bump

remove the elementType prop

Ideally, we would also provide a codemod to run the migration.

I think this is what was aimed for here and also agreed upon before. Generally, I see the intend here to warn through prop-types given the prop is used. Then fix up the warnings (while still supporting the prop) by migrating to as and then in the next major dropping the elementType.

How did you read us wanting a quick breaking change from this code?

I think providing a codemod is a bit overkill considering we could make the changes in the MC-FE in about 2 minutes with find and replace.

I think the path described by @tdeekens seems quite reasonable.

Yeah, would be a nice learning experience to write a codemod during tech time if anybody wants but seems faster without. Assuming that not too many people use ui-kit yet (whom the find and replace is also quick an easy).

I am just lagging behind to how the suggested implementation here seemed to suggest another path than outlined above.

emmenko

To be honest, I think there are several implications that need to be discussed first instead of jumping into the implementation.

I appreciate the thoughts and effort put into this but I would like to have an issue first where we discuss the usage and the API around this, as I feel it has bigger implications for the text components.

emmenko · 2019-04-03T13:26:25Z

src/components/typography/text/text.js

+        ...rest
+      );
+    }
+    return PropTypes.oneOf(['h1', 'h2', 'h3'])(


I'm a bit confused now. What's the purpose of changing the prop elementType to as if you have the same exact restrictions as before? 🤔

...to settle on a convention most other component libraries also have in offeing a prop called as not elementType. Other components, before we started with #628 assuming there too, would then offer an as prop too if needed and not elementType.

To be honest, I think there are several implications that need to be discussed first instead of jumping into the implementation.

We have an issue where the prop name was suggested. We (three developers) have discused this before starting the implementation assuming that there was no arhm. Anyway, assuming that there is, lets move the discussion there. Can you lay out the specific bigger implication you have for text components there?

tdeekens

Thanks for giving this a spin. I really value trying to have offer an as prop instead of elementType with a deprecation migration plan.

src/components/typography/text/text.js

tdeekens · 2019-04-03T14:21:13Z

src/components/typography/text/text.js

+      }
+    }
+
+    return PropTypes.bool(props, propName, componentName, ...rest);


Idea. Maybe we can share these prop-types in typography/prop-types. I know that they will be removed but it makes the actual code less noisy.

jonnybel · 2019-05-06T13:42:00Z

Thinking about merging this into 10.0.0, since it already implies breaking changes.

I'm updating the branch and we can discuss this further.

jonnybel · 2019-05-06T14:25:21Z

I have a few things I'd like to discuss about the Text.Detail component and some positive implications of changing it a tiny bit.

The isInline prop is a bit counter-intuitive... Because the <small> element is inline by default and is supposed to be used inside a body text. If we want to use it as a block, I think it should be a p with different font size instead 🤔 So, in the end, I think it should either be removed or the logic inverted. (I prefer removing it since we're also removing isInline for all the other Text components).
It has a className prop. I searched on both the ui-kit and merchant-center-frontend but couldn't find any usage of it... Also, it's weird that it's the only Text component that has this prop. So it could also be removed?

As such, given these changes, almost all Text components (except for Wrap) could compose on top of the same "BaseText" component to reduce repetition of code, as they have almost the same API and mostly just accept different values as props.
For instance, the Text.Detail component is simply a Text as a <small> element, and the rest is all the same as the Text.Body.

Thoughts on this?

montezume · 2019-05-06T14:46:22Z

@jonnybel

Yeah, seems to me like the usage of isInline could just be replaced with a wrapping div. This makes more sense to me 🤔. I would remove it.
Yes we should remove the className prop.

montezume · 2019-05-08T11:01:01Z

The diff from the icon refactoring is showing in this PR even though this is aimed at 10.0.0. Can you rebase it maybe so it fixes that?

jonnybel · 2019-05-08T11:02:42Z

@montezume Done!

montezume

Looks good to me now for 10.0.0

jonnybel · 2019-05-10T13:38:03Z

@emmenko anything else you'd like to have changed?

We can use the same codemod I've added to #747 to automate the refactoring of these props!
This PR could be added to our v10 project, since it also implies breaking changes.

emmenko

Nice, I like it!! 💯

Thanks a lot also for writing the codemod, pretty dope! 🙌

src/components/typography/text/text.js

jonnybel · 2019-05-13T15:08:42Z

Ok, the VRT detected changes now are only the prop name changes, in addition to the ones inherited from 10.0.0.

* feat(text): add as prop * chore: update specs and story * chore: update readme * refactor: use requiredIf * refactor: elementType to as * refactor: remove className prop from Text.Details component * refactor(text): use util for deprecation warning * fix(tag): style text with css selector after removal of classname prop

jonnybel added the 🚧 Status: WIP Work in progress label Apr 3, 2019

jonnybel requested review from emmenko, tdeekens and montezume April 3, 2019 13:19

jonnybel force-pushed the ja-text-add-as-prop branch from 635d31b to c40685b Compare April 3, 2019 13:19

emmenko requested changes Apr 3, 2019

View reviewed changes

emmenko reviewed Apr 3, 2019

View reviewed changes

tdeekens changed the title ~~WIP: feat(text): Add 'as' prop~~ WIP: feat(text): Add as prop while deprecating elementType Apr 3, 2019

tdeekens reviewed Apr 3, 2019

View reviewed changes

jonnybel force-pushed the ja-text-add-as-prop branch from 00c334e to 73e7a79 Compare May 6, 2019 12:35

jonnybel force-pushed the ja-text-add-as-prop branch from 73e7a79 to 50ba651 Compare May 6, 2019 13:43

jonnybel changed the base branch from master to 10.0.0 May 6, 2019 13:43

jonnybel force-pushed the ja-text-add-as-prop branch from 50ba651 to 6c68f3f Compare May 6, 2019 13:54

jonnybel marked this pull request as ready for review May 6, 2019 14:11

montezume force-pushed the 10.0.0 branch from a09b3f9 to 5baf7e6 Compare May 6, 2019 15:22

jonnybel force-pushed the ja-text-add-as-prop branch from 6c68f3f to 077ae53 Compare May 7, 2019 12:06

montezume force-pushed the 10.0.0 branch from 5baf7e6 to 88a5cb1 Compare May 8, 2019 08:02

jonnybel force-pushed the ja-text-add-as-prop branch from 38594d4 to 0b9eb76 Compare May 8, 2019 10:49

jonnybel force-pushed the ja-text-add-as-prop branch from 0b9eb76 to c59f17e Compare May 8, 2019 11:02

jonnybel changed the title ~~WIP: feat(text): Add as prop while deprecating elementType~~ feat(text): Add as prop while deprecating elementType May 8, 2019

montezume approved these changes May 8, 2019

View reviewed changes

jonnybel removed the 🚧 Status: WIP Work in progress label May 8, 2019

jonnybel force-pushed the ja-text-add-as-prop branch from c59f17e to 4cd45fb Compare May 9, 2019 08:54

jonnybel mentioned this pull request May 9, 2019

refactor: Text Inputs to spread attributes and remove wrapper #747

Merged

emmenko approved these changes May 10, 2019

View reviewed changes

src/components/typography/text/text.js Outdated Show resolved Hide resolved

tdeekens approved these changes May 10, 2019

View reviewed changes

jonnybel added 6 commits May 13, 2019 15:05

feat(text): add as prop

a52daad

chore: update specs and story

841f9a7

chore: update readme

093f78d

refactor: use requiredIf

2015da0

refactor: elementType to as

5207f80

refactor: remove className prop from Text.Details component

933d3f3

jonnybel force-pushed the ja-text-add-as-prop branch from 4cd45fb to f5f938b Compare May 13, 2019 13:09

refactor(text): use util for deprecation warning

dcff062

jonnybel force-pushed the ja-text-add-as-prop branch from f5f938b to dcff062 Compare May 13, 2019 13:36

fix(tag): style text with css selector after removal of classname prop

97ccdb6

jonnybel merged commit 538ea3f into 10.0.0 May 13, 2019

tdeekens deleted the ja-text-add-as-prop branch May 13, 2019 15:13

montezume mentioned this pull request Jun 21, 2019

Version 10.0.0 #877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(text): Add `as` prop while deprecating `elementType` #631

feat(text): Add `as` prop while deprecating `elementType` #631

jonnybel commented Apr 3, 2019 •

edited

Loading

emmenko Apr 3, 2019

tdeekens Apr 3, 2019

montezume Apr 3, 2019

tdeekens Apr 3, 2019

emmenko left a comment

emmenko Apr 3, 2019

tdeekens Apr 3, 2019

tdeekens Apr 3, 2019 •

edited

Loading

tdeekens left a comment

tdeekens Apr 3, 2019

jonnybel commented May 6, 2019

jonnybel commented May 6, 2019 •

edited

Loading

montezume commented May 6, 2019

montezume commented May 8, 2019

jonnybel commented May 8, 2019

montezume left a comment

jonnybel commented May 10, 2019

emmenko left a comment

jonnybel commented May 13, 2019

feat(text): Add as prop while deprecating elementType #631

feat(text): Add as prop while deprecating elementType #631

Conversation

jonnybel commented Apr 3, 2019 • edited Loading

Summary

Questions

emmenko Apr 3, 2019

Choose a reason for hiding this comment

tdeekens Apr 3, 2019

Choose a reason for hiding this comment

montezume Apr 3, 2019

Choose a reason for hiding this comment

tdeekens Apr 3, 2019

Choose a reason for hiding this comment

emmenko left a comment

Choose a reason for hiding this comment

emmenko Apr 3, 2019

Choose a reason for hiding this comment

tdeekens Apr 3, 2019

Choose a reason for hiding this comment

tdeekens Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

tdeekens left a comment

Choose a reason for hiding this comment

tdeekens Apr 3, 2019

Choose a reason for hiding this comment

jonnybel commented May 6, 2019

jonnybel commented May 6, 2019 • edited Loading

montezume commented May 6, 2019

montezume commented May 8, 2019

jonnybel commented May 8, 2019

montezume left a comment

Choose a reason for hiding this comment

jonnybel commented May 10, 2019

emmenko left a comment

Choose a reason for hiding this comment

jonnybel commented May 13, 2019

feat(text): Add `as` prop while deprecating `elementType` #631

feat(text): Add `as` prop while deprecating `elementType` #631

jonnybel commented Apr 3, 2019 •

edited

Loading

tdeekens Apr 3, 2019 •

edited

Loading

jonnybel commented May 6, 2019 •

edited

Loading