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(odyssey-react-mui): add overline typography variant #2349

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bryancunningham-okta
Copy link
Contributor

OKTA-803564

Summary

  • Adds Overline typography component
  • Only allow all caps when lang is 'en'

Testing & Screenshots

  • I have confirmed this change with my designer and the Odyssey Design Team.

@bryancunningham-okta bryancunningham-okta requested a review from a team as a code owner September 11, 2024 14:00
benlister-okta
benlister-okta previously approved these changes Sep 16, 2024
},
render: (args) => <Overline {...args} children={args.children} />,
play: async ({}) => {
await axeRun("Typopgraphy Overline");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await axeRun("Typopgraphy Overline");
await axeRun("Typography Overline");

},
render: (args) => <Legend {...args} children={args.children} />,
play: async ({}) => {
await axeRun("Typopgraphy legend");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await axeRun("Typopgraphy legend");
await axeRun("Typography legend");

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Comment on lines 3129 to 3135
"html:lang(en) &": {
textTransform: "uppercase",
},

"html:lang(en-*) &": {
textTransform: "uppercase",
},
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta Sep 16, 2024

Choose a reason for hiding this comment

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

Do we want this instead since we always want these to share changes?

Suggested change
"html:lang(en) &": {
textTransform: "uppercase",
},
"html:lang(en-*) &": {
textTransform: "uppercase",
},
"html:lang(en) &, html:lang(en-*) &": {
textTransform: "uppercase",
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KevinGhadyani-Okta Strangely, this doesn't work. I had to make 2 declarations for this to work correctly. I don't know why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to combine these selectors in quite a few ways to get it working. No luck

import { Typography } from "./Typography";

describe("Typography", () => {
test("renders overline", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test("renders overline", () => {
test("renders Overline", () => {

Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta 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 adding unit tests! Not much to test in this component since it's visual, and VRTs will cover the other half of it 👍.

Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

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

Approved from a dev perspective. I want someone from design to look at the Storybook and approve as well.

@bryancunningham-okta
Copy link
Contributor Author

Approved from a dev perspective. I want someone from design to look at the Storybook and approve as well.

@KevinGhadyani-Okta Where do we find the Storybook preview URL now?

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

Successfully merging this pull request may close these issues.

3 participants