🪟 🎨 Add Text component #15570

edmundito · 2022-08-11T20:04:24Z

What

Add <Text /> component to render text that matches the design system. Updates the auth pages as an example of how to use it.

Properties include:

as - h1, h2... or p, span, div
centered - if the text needs to be centered
bold - does not apply to headings
className

Usage:

<Text as="h1" size="lg">This is a large heading</Text>
<Text>This is regular text</Text>
<Text as="p">This is a paragarph</Text>
<Text size="lg" bold>This is large bold text</Text>

How

Component implementation is split to accept properties for a heading vs. regular text so that the use is simplified to one component. The styles are split into two modules because this allows applying style more easily override in components that apply class names to this and keeps the implementation cleaner.

Tests

Added storybook
Verified with auth pages
Verified that styles can be overridden without specificity

airbyte-webapp/src/components/base/Text/Text.tsx

krishnaglick · 2022-08-11T21:02:46Z

airbyte-webapp/src/components/base/Text/Text.tsx

+};
+
+export const Text: React.FC<TextProps> = ({
+  as = "span",


Do we have any data on what our default text container is? Should it be <p />?

That's a good question I wanted to ask! I'm not sure what the default element style should be. I considered paragraph as one of them; maybe that's the one that makes the most sense.

airbyte-webapp/src/components/base/Text/Text.tsx

edmundito · 2022-08-12T14:21:08Z

airbyte-webapp/src/components/base/Text/Text.tsx

+type TextElementType = "h1" | "h2" | "h3" | "h4" | "h5" | "h6" | "p" | "span" | "div";
+
+interface TextProps {
+  as?: TextElementType;


I was inspired by styled components to call this as but is the name clear? Alternatively, I thought of element or type as well. Thoughts?

as feels fairly standard, though some other opinions would definitely be useful :)

I like as here

++ for as as well, as it feels pretty standard as of today.

edmundito · 2022-08-15T20:58:53Z

Paired with @krishnaglick to resolve some issues regarding styling, updated the component to distinguish props between heading and text, and used the sign in pages as samples for using the text components.

krishnaglick

I paired w/ Edmundo for some of this so we should get another pair of 👀
But LGTM

Co-authored-by: Krishna Glick <krishna@airbyte.io>

edmundito · 2022-08-17T14:09:14Z

Updated the heading line-heights after reviewing with Nico, which should be the last change. I would like to get another review from someone else in @airbytehq/frontend before I call this done.

lmossman

Just had a couple of small questions, but otherwise I think this looks great!

lmossman · 2022-08-17T18:35:42Z

airbyte-webapp/src/components/base/Text/Text.tsx

+type TextElementType = "h1" | "h2" | "h3" | "h4" | "h5" | "h6" | "p" | "span" | "div";
+
+interface TextProps {
+  as?: TextElementType;


I like as here

lmossman · 2022-08-17T18:41:46Z

airbyte-webapp/src/components/base/Text/Text.tsx

+
+export const Text: React.FC<TextProps | HeadingProps> = React.memo((props) => {
+  const isHeading = isHeadingType(props);
+  const { as = isHeading ? "h1" : "p", centered = false, children } = props;


as = isHeading ? "h1" : "p"

This was slightly confusing to me, because isHeading will only ever be true if props.as is set, and its value is like hX. So it seems like we don't need to have a condition in the default value of as for when isHeading is true, because if it is true then we know props.as is set, so the default value will not be used.

Therefore, maybe it make sense to just set the default value here to p? Lmk if I'm misunderstanding anything

This is a good point. Probably Edmundo just coding defensively!

Good catch. Typing is a bit tricky here if we don't check for the heading type, that's why it partially destructuring the props. However, I realized that this is not needed and we can indeed assign 'p' to it.

lmossman · 2022-08-17T18:52:35Z

airbyte-webapp/src/components/base/Text/Text.tsx

+  size?: HeadingSize;
+}
+
+const getTextClassNames = ({ size, centered, bold }: Required<Pick<TextProps, "size" | "centered" | "bold">>) => {


Required<Pick<TextProps, "size" | "centered" | "bold">>

To confirm my understanding - the purpose of this is basically to remove the as and className fields from TextProps, because we don't actually want those to be class names on the component that we are returning below, correct?

If so, would it potentially be better to do something like Required<Omit<TextProps, "className" | "as">> instead, so that if we add more classname props to those types in the future we don't need to also edit these function signatures? Probably not a huge deal either way

You'd need to add the new property to the destructure anyway.
No matter what method, there's type safety if size, centered, or bold are changed in TextProps.

In case that the props were to expand, this would continue to work with Pick over using Omit.

Ah, good point that the new props would need to be added to the destructure, I forgot about that. This looks good to me then!

… `as` is required to infer heading

* Add Text component * Fix regexp for heading matching in Text component Co-authored-by: Krishna Glick <krishna@airbyte.io> * Reset margin and padding on text elements * Update default text element to p * Split scss for heading and text * Split types between Text and Heading * Rename text scss * Rename text scsss module * Remove font-weight from FormTitle * Split Text md size out of default style, fix typing issues * Update line heights on headings and remove override from SignupPage * Remove default heading type assignment when destructuring props since `as` is required to infer heading Co-authored-by: Krishna Glick <krishna@airbyte.io>

edmundito changed the title ~~Add Text component~~ 🪟 🎨 Add Text component Aug 11, 2022

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Aug 11, 2022

krishnaglick reviewed Aug 11, 2022

View reviewed changes

edmundito commented Aug 12, 2022

View reviewed changes

edmundito force-pushed the edmundito/text-component branch 2 times, most recently from 5c38263 to 30acdc6 Compare August 15, 2022 20:56

edmundito force-pushed the edmundito/text-component branch from 56fae12 to a8482a8 Compare August 16, 2022 13:30

edmundito marked this pull request as ready for review August 16, 2022 14:14

edmundito requested a review from a team as a code owner August 16, 2022 14:14

edmundito requested a review from krishnaglick August 16, 2022 14:22

krishnaglick approved these changes Aug 16, 2022

View reviewed changes

edmundito requested a review from a team August 16, 2022 19:22

edmundito and others added 11 commits August 17, 2022 10:02

Add Text component

caea515

Fix regexp for heading matching in Text component

8256b28

Co-authored-by: Krishna Glick <krishna@airbyte.io>

Reset margin and padding on text elements

9438826

Update default text element to p

cea10f8

Split scss for heading and text

bf2df7e

Split types between Text and Heading

7307790

Rename text scss

bbb58ce

Rename text scsss module

f50472c

Remove font-weight from FormTitle

69c6210

Split Text md size out of default style, fix typing issues

00d2c00

Update line heights on headings and remove override from SignupPage

74f66dd

edmundito force-pushed the edmundito/text-component branch from a8482a8 to 74f66dd Compare August 17, 2022 14:07

lmossman approved these changes Aug 17, 2022

View reviewed changes

Remove default heading type assignment when destructuring props since…

da8093e

… `as` is required to infer heading

edmundito merged commit 11583d3 into master Aug 18, 2022

edmundito deleted the edmundito/text-component branch August 18, 2022 16:15

octavia-squidington-iii mentioned this pull request Aug 19, 2022

Bump Airbyte version from 0.40.0-alpha to 0.40.1-alpha #15775

Closed

evantahler mentioned this pull request Aug 22, 2022

Bump Airbyte version from 0.40.0-alpha to 0.40.1 #15857

Merged

josephkmh mentioned this pull request Oct 14, 2022

🪟 🐛 Split Heading and Text components #17992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🎨 Add Text component #15570

🪟 🎨 Add Text component #15570

edmundito commented Aug 11, 2022 •

edited

Loading

krishnaglick Aug 11, 2022

edmundito Aug 12, 2022 •

edited

Loading

edmundito Aug 12, 2022

krishnaglick Aug 12, 2022

lmossman Aug 17, 2022

timroes Aug 17, 2022

edmundito commented Aug 15, 2022

krishnaglick left a comment

edmundito commented Aug 17, 2022

lmossman left a comment

lmossman Aug 17, 2022

lmossman Aug 17, 2022

krishnaglick Aug 17, 2022

edmundito Aug 17, 2022

lmossman Aug 17, 2022

krishnaglick Aug 17, 2022

edmundito Aug 17, 2022

lmossman Aug 17, 2022

🪟 🎨 Add Text component #15570

🪟 🎨 Add Text component #15570

Conversation

edmundito commented Aug 11, 2022 • edited Loading

What

How

Recommended reading order

Tests

Choose a reason for hiding this comment

edmundito Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito commented Aug 15, 2022

krishnaglick left a comment

Choose a reason for hiding this comment

edmundito commented Aug 17, 2022

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundito commented Aug 11, 2022 •

edited

Loading

edmundito Aug 12, 2022 •

edited

Loading