Skip to content
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

added a Text component #18495

Merged
merged 24 commits into from
Jan 7, 2020
Merged

added a Text component #18495

merged 24 commits into from
Jan 7, 2020

Conversation

jameslnewell
Copy link
Contributor

@jameslnewell jameslnewell commented Nov 14, 2019

Description

Added a Text mixin and component to @wordpress/components which provides typography styles in-line with the wp-admin-iterations experiment.

This addition provides a text mixin (e.g. text({ variant: 'body.small' })) that can be consumed in other components which are also using emotion (probably focussed more on internal usage) in addition to a Text component (e.g. <Text variant="body.small">).

This component is usable on web AND mobile, including styles.

To keep this PR small and easily reviewable I haven't included the following additions that I'll attempt to add in a future PR:

  • additional props for the Text component e.g. color, textAlign, margin, padding etc etc
  • start to compose the text mixin into other components in order to improve the consistency of typography across the components

Related PR: #18244

How has this been tested?

I've written a few tests for web and react-native and manually tested the web component in storybook. I'm not sure what the best approach is to manually test in react native before the package is published??

Screenshots

See the storybook.

Types of changes

New feature - mixins and a component for reusing/composing typography consistently

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards. - Yes, I think the biggest concern is the consuming developer using a semantic element which they can choose via the as prop
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@ItsJonQ ItsJonQ self-requested a review November 14, 2019 08:38
@ItsJonQ ItsJonQ added the [Package] Components /packages/components label Nov 14, 2019
package.json Show resolved Hide resolved
@jameskoster
Copy link
Contributor

Exciting!

in-line with the wp-admin-iterations experiment

I'm not 100% sure, but I think it might be better to use the type scale proposal that was submitted here.

There have been a couple of minor iterations, and it's pretty close to what you have. Latest Figma here and codepen here.

@ItsJonQ
Copy link

ItsJonQ commented Nov 14, 2019

@jameslnewell Thank you so much for adding this! 🎉 I left a couple of comments for you.

I am incredibly excited for this particular component. Something that renders text may seem trivial.. but there are many benefits as we start using this throughout the system. The folks within the native mobile world have already enjoyed the benefits of this.

In the future, I could see us enhancing this primitive with features like ellipsizeMode and perhaps common colour variants (e.g. muted).

@jameslnewell
Copy link
Contributor Author

@jameskoster I've updated the styles to match the code-pen. Can you please confirm they're current. 🙏

@jameslnewell
Copy link
Contributor Author

In the future, I could see us enhancing this primitive with features like ellipsizeMode and perhaps common colour variants (e.g. muted).

@ItsJonQ since the mobile variant is a Text it should already support ellipsizeMode???

Colors is something I definitely want to add in a followup PR! 👇

To keep this PR small and easily reviewable I haven't included the following additions that I'll attempt to add in a future PR:

  • additional props for the Text component e.g. color, textAlign, margin, padding etc etc
  • start to compose the text mixin into other components in order to improve the consistency of typography across the components

@jameslnewell jameslnewell requested a review from gziolo November 18, 2019 07:09
@ItsJonQ
Copy link

ItsJonQ commented Nov 21, 2019

Including Storybook screenshot for context:

Screen Shot 2019-11-21 at 9 33 12 AM

@ItsJonQ
Copy link

ItsJonQ commented Nov 21, 2019

@ItsJonQ since the mobile variant is a Text it should already support ellipsizeMode???

@jameslnewell Oh yes, I was talking about adding that feature for the web version of <Text /> 😉

@youknowriad
Copy link
Contributor

I have some questions here.

How are we planning to use this component?
Are we planning a Refactor to existing components?
Should we do it in one PR or iteratively?
When working on several components, how do we choose whether to use this component or just a regular div or p or whatever?
Do we foresee any performance impact if we start using this component everywhere?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the period with lots of time off, I completely forgot about this PR.

I think, this one is in good shape and it's an important bit leading towards validation of the CSS in JS approach. Let's get it in and see how it all works.

Copy link

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameslnewell Looks great! Thank you for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.