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

[FAD-57] - Add typography #8

Merged
merged 3 commits into from
Sep 7, 2022
Merged

[FAD-57] - Add typography #8

merged 3 commits into from
Sep 7, 2022

Conversation

luansievers
Copy link
Collaborator

@luansievers luansievers commented Sep 5, 2022

Description

This PR includes the dark mode on storybook:
image

Implemented typography Based on https://www.figma.com/file/g7Edk2JCOMLpRAw9v8NJ4c/FA-Product-Wireframe---working-file?node-id=29%3A770

BodyText

image

Caption

image

Display

image

Heading

image

Link

image

Checklist

  • CHANGELOG has been updated (if appropriate)
  • Environment variables updated (if appropriate)

Comment on lines +1 to +31
import React from "react";
import { DocsContainer as BaseContainer } from "@storybook/addon-docs/blocks";
import { useDarkMode } from "storybook-dark-mode";
import { themes } from "@storybook/theming";

export const DocsContainer = ({ children, context }) => {
const dark = useDarkMode();

return (
<BaseContainer
context={{
...context,
storyById: (id) => {
const storyContext = context.storyById(id);
return {
...storyContext,
parameters: {
...storyContext?.parameters,
docs: {
...storyContext?.parameters?.docs,
theme: dark ? themes.dark : themes.light,
},
},
};
},
}}
>
{children}
</BaseContainer>
);
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@sachasmart-weavik sachasmart-weavik Sep 6, 2022

Choose a reason for hiding this comment

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

lol what a url for a reference

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This component is a solution for dark mode with docs, apparently, they are both addons from the storybook and docs are not currently working with dark mode...

Comment on lines +76 to +81
<></>
) : // <HelperText className="mt-2">
// {/* @ts-expect-error Same as above */}
// Error: {errors[reservedErrorField].message}
// </HelperText>
null}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting for future references.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Food for thought - this reference is in client2, could we not just delete it outright?

Comment on lines +150 to +159
<></>
) : // <HelperText
// className={clsx(
// isError ? "text-state-error" : "text-dark-80",
// "mt-1 text-sm leading-none"
// )}
// >
// {_errorMessage ? _errorMessage : helperText}
// </HelperText>
null}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting for future references.

Comment on lines +191 to +204
) : // <HelperText
// className={clsx(
// isError
// ? "text-clay-500"
// : colorScheme === "light"
// ? "text-sand-500"
// : colorScheme === "dark"
// ? "text-sand-300"
// : null,
// "mt-1 text-sm leading-none"
// )}
// >
// {errorMessage ? errorMessage : helperText}
// </HelperText>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting for future references.

Comment on lines +99 to +101
<></>
) : // <HelperText isError>Error while fetching wallet status</HelperText>
null}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting it for future reference.

Comment on lines +24 to +28
{/* <Paragraph className="mb-8">
By connecting your wallet, you agree to our{" "}
<Link href="/terms">Terms of Service</Link> and{" "}
<Link href="/privacy">Privacy Policy</Link>
</Paragraph>
</Paragraph> */}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting it for future reference.

Comment on lines +71 to +76
<></>
) : // <HelperText isError className="mb-12">
// There was a problem fetching data on pools. Shown data may be
// outdated.
// </HelperText>
null}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting it for future reference.

Comment on lines +295 to +300
<></>
) : // <HelperText isError>
// There was a problem fetching data on this pool. Shown data may be
// outdated.
// </HelperText>
null}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting it for future reference.

Comment on lines +188 to +193
<></>
) : // <HelperText isError>
// There was a problem fetching data on the senior pool. Shown data
// may be outdated.
// </HelperText>
null}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Old component used by goldfinch. I commented instead of deleting it for future reference.

@luansievers luansievers changed the title Add typography [FAD-57] - Add typography Sep 6, 2022
Comment on lines +76 to +81
<></>
) : // <HelperText className="mt-2">
// {/* @ts-expect-error Same as above */}
// Error: {errors[reservedErrorField].message}
// </HelperText>
null}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Food for thought - this reference is in client2, could we not just delete it outright?

Copy link
Collaborator

@v1n33th v1n33th left a comment

Choose a reason for hiding this comment

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

Awesome..Just a few questions.

className,
...rest
}: BodyTextProps) {
const Component = "p";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just asking, is there any reasoning for declaring the component at the top?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +23 to +29
level === 1
? "font-bold"
: level < 5
? "font-semibold"
: medium
? "font-medium"
: "font-semibold",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest using objects when there are multiple ternary operations involved.
Something like this,
clsx("font-semibold",{"font-semibold":level < 5,"font-bold":level === 1,"font-medium":medium},className)

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not quite familiar with this, but I've tested, and doing this way results in wrong results with the fonts. such as:
font-semibold font-semibold or font-semibold font-bold

@luansievers luansievers merged commit f7e4c4e into main Sep 7, 2022
@luansievers luansievers deleted the FAD-57-typography branch September 7, 2022 10:22
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