Skip to content

Conversation

@AlSirang
Copy link
Contributor

@AlSirang AlSirang commented Oct 21, 2024

Consolidate Heading Components into a Single Text Component

This pull request consolidates individual heading components (H1, H2, H3, H4, H5, and H6) into a single, flexible Text component. This approach streamlines the implementation of headings while maintaining semantic HTML and accessibility.

Features:

  • Single Text Component: A unified component that allows users to specify the desired heading level (from h1 to h6 and p) using props, while applying the appropriate styles for each level.
  • Variant Support: Utilizes class-variance-authority to handle styling variants, ensuring consistent typography across different heading levels.
  • Flexible Props: Supports className, component, and additional HTML attributes, allowing for extensive customization and integration.
  • Improved Documentation: Updated documentation includes usage examples and guidelines for the new consolidated component, making it easier for developers to adopt.

Usage Example:

import { Text } from "./Text";

const ExampleComponent = () => {
  return (
    <div>
      <Text as="h1">This is an H1 Heading</Text>
      <Text as="h2">This is an H2 Heading</Text>
      <Text as="h3">This is an H3 Heading</Text>
      <Text as="h4">This is an H4 Heading</Text>
      <Text as="h5">This is an H5 Heading</Text>
      <Text as="h6">This is an H6 Heading</Text>
      <Text>This is a paragraph Element</Text>
    </div>
  );
};

@AlSirang AlSirang changed the title Fix/merge typography components Consolidate Heading Components into a Single Typography Component Oct 21, 2024
@ariflogs ariflogs added the enhancement New feature or request label Oct 21, 2024
@ariflogs
Copy link
Member

ariflogs commented Oct 21, 2024

Good idea. thanks for the great work.
But don't you think naming the component something like Heading would be better?

@AlSirang
Copy link
Contributor Author

Yes, we could have used a component name like Heading, but in the future, we can add a paragraph to the Typography component to cover all text tags.

@ariflogs
Copy link
Member

like every typography types(heading, p, quotes, code...) all in the same component?
that might start to get a little confusing for the users.

@ariflogs
Copy link
Member

btw are you in our Discord server?

@AlSirang
Copy link
Contributor Author

like every typography types(heading, p, quotes, code...) all in the same component?
that might start to get a little confusing for the users.

No, just h1 to h6 and p tag. Not all of them. Just like the MUI Typography component.

@AlSirang
Copy link
Contributor Author

btw are you in our Discord server?
No, not yet.

@ariflogs
Copy link
Member

makes sense. I started a poll on the community server. let's see if others have any opinion on this.

Copy link
Member

@ariflogs ariflogs Oct 22, 2024

Choose a reason for hiding this comment

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

Looks like we don't need both variant and tag, let's combine both and make it as

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for both the tag and variants is to allow users to apply sizes and styles from other elements to the specified tag. If we remove the variants, we will lose that feature.

Copy link
Member

Choose a reason for hiding this comment

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

you mean to let users do something like this <Text variant="h2" tag="p" />?


const textVariants = cva("font-head", {
variants: {
variant: {
Copy link
Member

@ariflogs ariflogs Oct 22, 2024

Choose a reason for hiding this comment

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

variants: {
  as: {
    p: ""
    }
....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean I should update the variant prop to as?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Member

@ariflogs ariflogs left a comment

Choose a reason for hiding this comment

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

Thanks for the great work 🚀

}

export const Text = <T extends TagsMap>(props: TextProps<T>) => {
const { className, tag = "p" as T, variant, ...otherProps } = props;
Copy link
Member

Choose a reason for hiding this comment

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

as = "p" as T

Copy link
Member

Choose a reason for hiding this comment

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

you mean to let users do something like this <Text variant="h2" tag="p" />?


const textVariants = cva("font-head", {
variants: {
variant: {
Copy link
Member

Choose a reason for hiding this comment

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

yes

@ariflogs ariflogs merged commit 062eccd into Logging-Studio:main Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Released 🚀

Development

Successfully merging this pull request may close these issues.

2 participants