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

feat:(Typography) add text-align prop and support span variant <FE-6204 & FE-6220> #6334

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

nineteen88
Copy link
Contributor

@nineteen88 nineteen88 commented Sep 28, 2023

Proposed behaviour

  • add prop support for text-align
  • include support for using span as a variant

fixes #6331

Current behaviour

Currently there is no support for the above props and span variant

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Unit tests added or updated if required
  • Storybook added or updated if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in CodeSandbox/storybook
  • Add new Cypress test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

https://codesandbox.io/s/carbon-quickstart-forked-24w5sc

@nineteen88 nineteen88 force-pushed the FE-6204-Typography-support-span-variant branch from baff024 to c46b2f6 Compare September 28, 2023 15:33
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 28, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 553d854:

Sandbox Source
carbon-quickstart Configuration
carbon-quickstart-typescript Configuration
carbon-quickstart PR
carbon-quickstart (forked) PR

@nineteen88 nineteen88 force-pushed the FE-6204-Typography-support-span-variant branch from c46b2f6 to 85809b6 Compare September 29, 2023 14:02
@Parsium Parsium self-requested a review October 2, 2023 14:11
src/components/typography/typography.component.tsx Outdated Show resolved Hide resolved
src/components/typography/typography.spec.tsx Outdated Show resolved Hide resolved
src/components/typography/typography.spec.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@tomdavies73 tomdavies73 left a comment

Choose a reason for hiding this comment

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

suggestion: we have some variant tests already in typography.cy.tsx, where we check the variant is applied, font-size, line-height, font-weight, text-transform and text-decoration-line. It might be worth adding "span" to these tests. We can do this by adding "span" to VARIANT_TYPES, getAs, getSize, getLineHeight, getWeight functions etc. Could be a good shout to ensure span is added with other variants as a baseline.

@nineteen88 nineteen88 force-pushed the FE-6204-Typography-support-span-variant branch 2 times, most recently from 1622ffd to 1582c73 Compare October 12, 2023 10:36
@nineteen88 nineteen88 changed the title feat:(Typography) add tabindex, ref, text-align props and support span variant <FE-6204 & FE-6220> feat:(Typography) add text-align prop and support span variant <FE-6204 & FE-6220> Oct 12, 2023
@nineteen88
Copy link
Contributor Author

suggestion: we have some variant tests already in typography.cy.tsx, where we check the variant is applied, font-size, line-height, font-weight, text-transform and text-decoration-line. It might be worth adding "span" to these tests. We can do this by adding "span" to VARIANT_TYPES, getAs, getSize, getLineHeight, getWeight functions etc. Could be a good shout to ensure span is added with other variants as a baseline.

Added this now.

tomdavies73
tomdavies73 previously approved these changes Oct 12, 2023
Parsium
Parsium previously approved these changes Oct 12, 2023
@Parsium Parsium marked this pull request as ready for review October 12, 2023 10:55
@Parsium Parsium requested review from a team as code owners October 12, 2023 10:55
@divyajindel
Copy link
Contributor

Hi @nineteen88 , could you please add cypress tests for TextAlign prop?

@nineteen88 nineteen88 dismissed stale reviews from Parsium and tomdavies73 via 52cc415 October 17, 2023 08:59
@nineteen88 nineteen88 force-pushed the FE-6204-Typography-support-span-variant branch 2 times, most recently from 52cc415 to d7af436 Compare October 17, 2023 09:00
Copy link
Contributor

@Parsium Parsium Oct 17, 2023

Choose a reason for hiding this comment

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

note: unrelated to these changes, but it might be worth us refactoring the cypress file in the future to remove these value lookups on lines 12-160. I feel this could be simplified quite a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've spoken to @edleeks87 about this and we will do a refactor of the tests later. You're definitely correct.

@nineteen88 nineteen88 force-pushed the FE-6204-Typography-support-span-variant branch from d7af436 to 553d854 Compare October 17, 2023 15:00
@nineteen88 nineteen88 merged commit 2662239 into master Oct 17, 2023
@nineteen88 nineteen88 deleted the FE-6204-Typography-support-span-variant branch October 17, 2023 15:45
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 121.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

textAlign attribute for Typography component
5 participants