Skip to content

Custom Typography HOC wrapper for Ant#6062

Merged
gilluminate merged 7 commits intomainfrom
gill/LJ-692/typography
Apr 18, 2025
Merged

Custom Typography HOC wrapper for Ant#6062
gilluminate merged 7 commits intomainfrom
gill/LJ-692/typography

Conversation

@gilluminate
Copy link
Copy Markdown
Contributor

@gilluminate gilluminate commented Apr 17, 2025

Closes LJ-692

Description Of Changes

Introduces a new custom Typography HOC in FidesUI that wraps Ant Design's Typography component, adding support for custom text sizes and heading sizes.

Code Changes

  • Added new CustomTypography HOC in FidesUI with support for custom text sizes (sm, default, lg) and heading sizes (h1-h5)
  • Added corresponding SCSS styles for typography size customization as well as custom paragraph margin handling.
  • Updated FidesUI exports to use the new CustomTypography component
  • Updated theme configuration with new typography-related token values (H1, H2, H3, etc)
  • Added Typography showcase section in the Ant POC page
  • Migrated PageHeader component from Chakra's Heading to Ant's Typography.Title (since that's used in the Ant POC page anyway)

Steps to Confirm

  1. Check the Ant POC page to verify typography styles and variants

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
  • Followup issues:
    • Followup issues created (include link)
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link
Copy Markdown
Contributor

vercel Bot commented Apr 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 18, 2025 5:03pm
fides-privacy-center ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 18, 2025 5:03pm

<Flex justify="space-between">
{typeof heading === "string" ? (
<Heading
<Title
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since this component shows up in the Ant POC, I chose to migrate here. We'll save full migration for later

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good choice for the migration! I did find that the page titles changed from bold to semibold.
I tried switching it to 700 and the page titles stay consistent, but then the H2s, H3s become heavier than we had before. IMO the titles look better with 600 font-weight, but we ask for design input.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

update: design said 600 font weight is good!

<Content className="overflow-auto px-10 py-6">
<PageHeader heading="Ant Design Proof of Concept" />
<Row gutter={16}>
<Row gutter={16} className="mt-6">
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The header change made this necessary

variant="borderless"
className="h-full"
>
<Typography>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that this creates an <article> container when rendered

@gilluminate gilluminate marked this pull request as ready for review April 17, 2025 23:10
Copy link
Copy Markdown
Contributor

@lucanovera lucanovera 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 unblocking us with the typography issues. The changes look good, I appreciate the flexibility of overriding styles while keeping semantics the same.
I updated a couple uses of h5 that now should be h3's and added back the margin.
I also found that the title font weight changes from 700 to 600 while switching the page title from chakra to ant, let's ask for design feedback.

Other than that, all the code changes look good and found no other issues while testing so I'll leave my approve here. 🚀

>
{title}
</Typography.Title>
<Typography.Title level={3}>{title}</Typography.Title>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I pushed this change to change the title level to 3 and removed unnecessary styles.


import { NextBreadcrumb, NextBreadcrumbProps } from "./nav/NextBreadcrumb";

const { Title } = Typography;
Copy link
Copy Markdown
Contributor

@lucanovera lucanovera Apr 18, 2025

Choose a reason for hiding this comment

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

Ant uses this kind of destructuring in their examples, and it's nice using <Title> instead of Typography.Something every time you need to put a text.
But at the same time, do we want to have to add this line every single time we want to use a typography? Seems to me like the kind of thing that would get annoying after a while.

What do you think about exporting Title, Paragraph, Text, etc directly from fidesui? Can you see any downsides of doing that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of this approach also, provided there isn't an issue with it I'm missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Love the idea. I'll make that update

<Flex justify="space-between">
{typeof heading === "string" ? (
<Heading
<Title
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good choice for the migration! I did find that the page titles changed from bold to semibold.
I tried switching it to 700 and the page titles stay consistent, but then the H2s, H3s become heavier than we had before. IMO the titles look better with 600 font-weight, but we ask for design input.

data-testid="custom-fields-form"
>
<Typography.Title level={5}>Custom fields</Typography.Title>
<div className="mb-2">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I added this change to bring this title down to a h3 and added the margin back.

>
<div className="mb-4">
<Typography.Title level={5}>Details</Typography.Title>
<div className="mb-2">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I added this change to bring this title down to a h3 and added the margin back.

fontSizeXL: 24,
fontSizeLG: 18,
fontSizeSM: 12,
titleMarginBottom: 0,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In case you want to test title font weights, the token is fontWeightStrong: 700,

}: React.ComponentProps<typeof Typography.Paragraph> &
CustomTypographyTextProps) => (
<Typography.Paragraph
role="paragraph"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lucanovera @jpople I forgot to call this out before. There's no real way to change the rendered tag in the HOC, and on top of that, having it be a div was a choice made to better support the other variants like editable, etc. So instead of trying to wrangle all of that, I simply gave it the proper a11y role here and moved on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@gilluminate that's a good workaround for accessibility! With admin-ui we don't have to worry about SEO so I think this it okay.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True! But also, what's good for a11y is good for SEO ;)

@@ -1,4 +1,4 @@
import { AntFlex as Flex, Heading } from "fidesui";
import { AntFlex as Flex, AntTitle as Title } from "fidesui";
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lucanovera @jpople updated!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💯

@gilluminate gilluminate merged commit 5e74472 into main Apr 18, 2025
17 checks passed
@gilluminate gilluminate deleted the gill/LJ-692/typography branch April 18, 2025 17:18
@cypress
Copy link
Copy Markdown

cypress Bot commented Apr 18, 2025

fides    Run #12836

Run Properties:  status check passed Passed #12836  •  git commit 5e744725d1: Custom Typography HOC wrapper for Ant (#6062)
Project fides
Branch Review main
Run status status check passed Passed #12836
Run duration 00m 51s
Commit git commit 5e744725d1: Custom Typography HOC wrapper for Ant (#6062)
Committer Jason Gill
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 5
View all changes introduced in this branch ↗︎

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