这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@sensiblefolk
Copy link

Description

Converted Tab & AceEditor components from javascript to typescript and added type declarations

Changelog

  • CHANGELOG.md is updated with user-facing content relevant to this PR.

Affected components

  • Console

Related Issues

Related issue #4134

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No

Metadata

Does this PR add a new Metadata feature?

  • No

GraphQL

  • No new GraphQL schema is generated

Breaking changes

  • No Breaking changes

@sensiblefolk sensiblefolk requested a review from a team as a code owner April 23, 2020 15:26
@hasura-bot
Copy link
Contributor

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! 😎

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2020

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Apr 23, 2020

Deploy preview for hasura-docs ready!

Built with commit 0bbcd6a

https://deploy-preview-4536--hasura-docs.netlify.app

@hasura-bot
Copy link
Contributor

Review app for commit 0bbcd6a deployed to Heroku: https://hge-ci-pull-4536.herokuapp.com
Docker image for server: hasura/graphql-engine:pull4536-0bbcd6a5

Copy link
Contributor

@beerose beerose 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 your PR! 🎉

Ace Editor code looks good.
Regarding Tabs I have some comments.

  1. Let's keep typings close to the components and remove typings.ts file.

  2. type ITabListItemDefaultProps = Readonly<typeof styledTabListItemDefaultProps>; is not needed. Could you move default props back to Tabs.tsx?

  3. We try to stick to the convention of not prefixing interfaces with I.

  4. We need to provide more exhaustive typings for Tabs and StyledTabs.

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 Box and interface BoxProps in Tabs.ts. Another PR adds it to the separate folder, but as it's not yet merged, we can keep it Tabs.ts file.
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 Box and StyledTabOwnProps to declare StyledTab:
const StyledTab = styled(Box)<StyledTabOwnProps>`
  ${color}
  ${border}
  ${typography}
  ${layout}
  ${space}
  ${shadow}
`;
  • Export interface StyledTabProps that extends both tab's own props (styled) and HTML div element attributes.
export interface StyledTabProps extends StyledTabOwnProps, BoxProps {}
  • Use newly created interface to declare TabsProps in index.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';
Copy link
Contributor

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.

Suggested change
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

Suggested change
export const ACE_EDITOR_FONT_SIZE: number = 14;
export const ACE_EDITOR_FONT_SIZE = 14;

@beerose beerose mentioned this pull request May 29, 2020
46 tasks
@beerose
Copy link
Contributor

beerose commented Jun 24, 2021

Closing this PR due to lack of activity

@beerose beerose closed this Jun 24, 2021
@hasura-bot
Copy link
Contributor

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants