-
Notifications
You must be signed in to change notification settings - Fork 2.8k
converted Tabs & AceEDitor components to typescript and added typings #4536
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
Conversation
|
Beep boop! 🤖 Hey @sensiblefolk, thanks for your PR! One of my human friends will review this PR and get back to you as soon as possible. Stay awesome! 😎 |
|
Deploy preview for hasura-docs ready! Built with commit 0bbcd6a |
|
Review app for commit 0bbcd6a deployed to Heroku: https://hge-ci-pull-4536.herokuapp.com |
beerose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR! 🎉
Ace Editor code looks good.
Regarding Tabs I have some comments.
-
Let's keep typings close to the components and remove
typings.tsfile. -
type ITabListItemDefaultProps = Readonly<typeof styledTabListItemDefaultProps>;is not needed. Could you move default props back toTabs.tsx? -
We try to stick to the convention of not prefixing interfaces with
I. -
We need to provide more exhaustive typings for
TabsandStyledTabs.
Right now Tabs accepts tabsData and [propsName: string]: any;. Let's narrow this type. It should accept tabsData, all style props accepted by StyledTab and all HTML div element attributes.
In order to have this, we should do the following:
- Crate component
Boxand interfaceBoxPropsin Tabs.ts. Another PR adds it to the separate folder, but as it's not yet merged, we can keep itTabs.tsfile.
interface BoxProps extends Omit<React.ComponentPropsWithRef<'div'>, 'color'> {}
const Box = ('div' as any) as React.FC<BoxProps>;We are doing this because styled-components and TypeScript doesn't work well together, and there are some type conflicts (the type of prop color from styled-system conflicts with div element's color.)
- Create an interface
StyledTabOwnProps:
export interface StyledTabOwnProps
extends ColorProps,
BorderProps,
FlexboxProps,
TypographyProps,
LayoutProps,
SpaceProps,
ShadowProps {}All the types used here are imported from styled-system.
- Use
BoxandStyledTabOwnPropsto declareStyledTab:
const StyledTab = styled(Box)<StyledTabOwnProps>`
${color}
${border}
${typography}
${layout}
${space}
${shadow}
`;- Export interface
StyledTabPropsthat extends both tab's own props (styled) and HTML div element attributes.
export interface StyledTabProps extends StyledTabOwnProps, BoxProps {}- Use newly created interface to declare
TabsPropsinindex.ts:
export interface TabsProps extends StyledTabProps {
tabsData: Array<{
title: string;
tabContent: any;
}>;
}
export const Tabs: React.FC<TabsProps> = props => { ...|
|
||
| export const ACE_EDITOR_THEME = 'eclipse'; | ||
| export const ACE_EDITOR_FONT_SIZE = 14; | ||
| export const ACE_EDITOR_THEME: string = 'eclipse'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As TypeScript can infer this.
| export const ACE_EDITOR_THEME: string = 'eclipse'; | |
| export const ACE_EDITOR_THEME = 'eclipse'; |
| export const ACE_EDITOR_THEME = 'eclipse'; | ||
| export const ACE_EDITOR_FONT_SIZE = 14; | ||
| export const ACE_EDITOR_THEME: string = 'eclipse'; | ||
| export const ACE_EDITOR_FONT_SIZE: number = 14; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
| export const ACE_EDITOR_FONT_SIZE: number = 14; | |
| export const ACE_EDITOR_FONT_SIZE = 14; |
|
Closing this PR due to lack of activity |
|
Beep boop! 🤖 Hey @sensiblefolk! Sorry that your PR wasn’t merged. Do take a look at any of the other open issues to see if you’d like to take something up! We’re around on Discord if you have any questions 😄 |
Description
Converted Tab & AceEditor components from javascript to typescript and added type declarations
Changelog
CHANGELOG.mdis updated with user-facing content relevant to this PR.Affected components
Related Issues
Related issue #4134
Limitations, known bugs & workarounds
Server checklist
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
GraphQL
Breaking changes