- Notifications
You must be signed in to change notification settings - Fork1k
fix(site): standardize headers for Admin Settings page#16911
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
20 commits Select commitHold shift + click to select a range
de69ee3 fix: migrate deployment workspace proxies page to new header style
Parkreinera1da1c1 fix: update other proxy page to use proper render key values
Parkreinere5d581b fix: make secondary styling more obvious
Parkreiner9b4928b refactor: move SettingsHeader to Tailwind
Parkreinera35a9ae fix: resolve header level bugs
Parkreiner9872c5d fix: constrain max width
Parkreiner1fe0022 fix: apply formatting
Parkreiner7d0903a fix: update HTML semantics
Parkreinera512bf5 fix: flush semantics fix
Parkreiner734f59d fix: update leading
Parkreiner49b9dfe fix: resolve styling edge case for beta badge
Parkreiner840d6df fix: set correct font size
Parkreinerf169c4e fix: remove redundant class
Parkreiner2325982 Merge branch 'main' into mes/headers-fix-b
Parkreiner99c2dbc refactor: switch to CVA
Parkreiner50eddd0 refactor: update API design of SettingsHeader
Parkreiner2b77784 chore: add first-ever stories for SettingsHeader
Parkreinere2523cb fix: make header more responsive
Parkreiner7f53666 fix: format
Parkreinerdaf6da9 fix: update beta badge styling for Notifications page
ParkreinerFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import type { Meta, StoryObj } from "@storybook/react"; | ||
| import { docs } from "utils/docs"; | ||
| import { | ||
| SettingsHeader, | ||
| SettingsHeaderDescription, | ||
| SettingsHeaderDocsLink, | ||
| SettingsHeaderTitle, | ||
| } from "./SettingsHeader"; | ||
| const meta: Meta<typeof SettingsHeader> = { | ||
| title: "components/SettingsHeader", | ||
| component: SettingsHeader, | ||
| }; | ||
| export default meta; | ||
| type Story = StoryObj<typeof SettingsHeader>; | ||
| export const PrimaryHeaderOnly: Story = { | ||
| args: { | ||
| children: <SettingsHeaderTitle>This is a header</SettingsHeaderTitle>, | ||
| }, | ||
| }; | ||
| export const PrimaryHeaderWithDescription: Story = { | ||
| args: { | ||
| children: ( | ||
| <> | ||
| <SettingsHeaderTitle>Another primary header</SettingsHeaderTitle> | ||
| <SettingsHeaderDescription> | ||
| This description can be any ReactNode. This provides more options for | ||
| composition. | ||
| </SettingsHeaderDescription> | ||
| </> | ||
| ), | ||
| }, | ||
| }; | ||
| export const PrimaryHeaderWithDescriptionAndDocsLink: Story = { | ||
| args: { | ||
| children: ( | ||
| <> | ||
| <SettingsHeaderTitle>Another primary header</SettingsHeaderTitle> | ||
| <SettingsHeaderDescription> | ||
| This description can be any ReactNode. This provides more options for | ||
| composition. | ||
| </SettingsHeaderDescription> | ||
| </> | ||
| ), | ||
| actions: <SettingsHeaderDocsLink href={docs("/admin/external-auth")} />, | ||
| }, | ||
| }; | ||
| export const SecondaryHeaderWithDescription: Story = { | ||
| args: { | ||
| children: ( | ||
| <> | ||
| <SettingsHeaderTitle level="h6" hierarchy="secondary"> | ||
| This is a secondary header. | ||
| </SettingsHeaderTitle> | ||
| <SettingsHeaderDescription> | ||
| The header's styling is completely independent of its semantics. Both | ||
| can be adjusted independently to help avoid invalid HTML. | ||
| </SettingsHeaderDescription> | ||
| </> | ||
| ), | ||
| }, | ||
| }; | ||
| export const SecondaryHeaderWithDescriptionAndDocsLink: Story = { | ||
| args: { | ||
| children: ( | ||
| <> | ||
| <SettingsHeaderTitle level="h3" hierarchy="secondary"> | ||
| Another secondary header | ||
| </SettingsHeaderTitle> | ||
| <SettingsHeaderDescription> | ||
| Nothing to add, really. | ||
| </SettingsHeaderDescription> | ||
| </> | ||
| ), | ||
| actions: <SettingsHeaderDocsLink href={docs("/admin/external-auth")} />, | ||
| }, | ||
| }; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Can we add a storybook for it covering the new variants? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,74 +1,107 @@ | ||
| import{typeVariantProps,cva}from"class-variance-authority"; | ||
| import{Button}from"components/Button/Button"; | ||
| import{SquareArrowOutUpRightIcon}from"lucide-react"; | ||
| importtype{FC,PropsWithChildren,ReactNode}from"react"; | ||
| import{cn}from"utils/cn"; | ||
| typeSettingsHeaderProps=Readonly< | ||
| PropsWithChildren<{ | ||
| actions?:ReactNode; | ||
| className?:string; | ||
| }> | ||
| >; | ||
| exportconstSettingsHeader:FC<SettingsHeaderProps>=({ | ||
| children, | ||
| actions, | ||
| className, | ||
| })=>{ | ||
| return( | ||
| <hgroupclassName="flex flex-col justify-between items-start gap-2 pb-6 sm:flex-row"> | ||
| {/* | ||
| * The text-sm class is only meant to adjust the font size of | ||
| * SettingsDescription, but we need to apply it here. That way, | ||
| * text-sm combines with the max-w-prose class and makes sure | ||
| * we have a predictable max width for the header + description by | ||
| * default. | ||
| */} | ||
| <divclassName={cn("text-sm max-w-prose",className)}>{children}</div> | ||
| {actions} | ||
| </hgroup> | ||
| ); | ||
| }; | ||
| typeSettingsHeaderDocsLinkProps=Readonly< | ||
| PropsWithChildren<{href:string}> | ||
| >; | ||
| exportconstSettingsHeaderDocsLink:FC<SettingsHeaderDocsLinkProps>=({ | ||
| href, | ||
| children="Read the docs", | ||
| })=>{ | ||
| return( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This was a big accessibility/HTML validity issue. We were using this component multiple times on the same page, but only one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This is why I decoupled the | ||
| <ButtonasChildvariant="outline"> | ||
| <ahref={href}target="_blank"rel="noreferrer"> | ||
| <SquareArrowOutUpRightIcon/> | ||
| {children} | ||
| <spanclassName="sr-only"> (link opens in new tab)</span> | ||
| </a> | ||
| </Button> | ||
| ); | ||
| }; | ||
| consttitleVariants=cva("m-0 pb-1 flex items-center gap-2 leading-tight",{ | ||
| variants:{ | ||
| hierarchy:{ | ||
| primary:"text-3xl font-bold", | ||
| secondary:"text-2xl font-medium", | ||
| }, | ||
| }, | ||
| defaultVariants:{ | ||
| hierarchy:"primary", | ||
| }, | ||
| }); | ||
| typeSettingsHeaderTitleProps=Readonly< | ||
| PropsWithChildren< | ||
| VariantProps<typeoftitleVariants>&{ | ||
| level?: `h${1|2|3|4|5|6}`; | ||
| tooltip?:ReactNode; | ||
| className?:string; | ||
| } | ||
| > | ||
| >; | ||
| exportconstSettingsHeaderTitle:FC<SettingsHeaderTitleProps>=({ | ||
| children, | ||
| tooltip, | ||
| className, | ||
| level="h1", | ||
| hierarchy="primary", | ||
| })=>{ | ||
| // Explicitly not using Radix's Slot component, because we don't want to | ||
| // allow any arbitrary element to be composed into this. We specifically | ||
| // only want to allow the six HTML headers. Anything else will likely result | ||
| // in invalid markup | ||
Comment on lines +78 to +81 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Added a comment to call out why the code isn't using | ||
| constTitle=level; | ||
| return( | ||
| <divclassName="flex flex-row gap-2 items-center"> | ||
| <TitleclassName={cn(titleVariants({ hierarchy}),className)}> | ||
| {children} | ||
| </Title> | ||
| {tooltip} | ||
| </div> | ||
| ); | ||
| }; | ||
| typeSettingsHeaderDescriptionProps=Readonly< | ||
| PropsWithChildren<{ | ||
| className?:string; | ||
| }> | ||
| >; | ||
| exportconstSettingsHeaderDescription:FC<SettingsHeaderDescriptionProps>=({ | ||
| children, | ||
| className, | ||
| })=>{ | ||
| return( | ||
| <pclassName={cn("m-0 text-content-secondary leading-relaxed",className)}> | ||
| {children} | ||
| </p> | ||
| ); | ||
| }; | ||
16 changes: 11 additions & 5 deletionssite/src/pages/DeploymentSettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
19 changes: 14 additions & 5 deletions...rc/pages/DeploymentSettingsPage/ExternalAuthSettingsPage/ExternalAuthSettingsPageView.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
17 changes: 12 additions & 5 deletionssite/src/pages/DeploymentSettingsPage/LicensesSettingsPage/AddNewLicensePageView.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
16 changes: 11 additions & 5 deletionssite/src/pages/DeploymentSettingsPage/LicensesSettingsPage/LicensesSettingsPageView.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.