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

feat(sidenav): update sidenav to typescript #917

Merged
merged 6 commits into from
May 18, 2020

Conversation

ivoreis
Copy link
Contributor

@ivoreis ivoreis commented May 11, 2020

Convert to typescript:

  • Sidenav

Relates to: #668

@ivoreis ivoreis mentioned this pull request May 11, 2020
32 tasks
@ivoreis ivoreis force-pushed the feat/typescript-sidenav branch 2 times, most recently from d18b4e2 to 6a3d71e Compare May 13, 2020 18:01
@ivoreis ivoreis marked this pull request as ready for review May 13, 2020 18:01
@ivoreis
Copy link
Contributor Author

ivoreis commented May 13, 2020

@hasparus Mind having a look please?

@ivoreis ivoreis force-pushed the feat/typescript-sidenav branch 2 times, most recently from 9646abe to 79484f6 Compare May 14, 2020 08:19
@hasparus hasparus self-assigned this May 14, 2020
packages/sidenav/src/index.tsx Outdated Show resolved Hide resolved
packages/css/src/types.ts Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
packages/sidenav/package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ivoreis ivoreis force-pushed the feat/typescript-sidenav branch 2 times, most recently from 7099a1f to a2b78b5 Compare May 18, 2020 07:24
@ivoreis ivoreis requested a review from hasparus May 18, 2020 07:24
Copy link
Member

@hasparus hasparus left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

I'll test it and merge it soon.

@hasparus
Copy link
Member

Okay, I'm merging it right now.
I got rid of as ReactNode assertions. It seems they're not needed.

as SxStyleProp can also be removed, but it needs a sort of weird syntax.

        <div
          {...props}
          ref={ref}
          sx={{
            position: ['fixed', 'sticky'],
            top: 0,
            left: 0,
            bottom: [0, 'auto'],
            zIndex: 1,
            minWidth: 0,
            width: 256,
            maxHeight: ['100vh', 'none'],
            overflowX: 'visible',
            overflowY: 'auto',
            transition: 'transform .2s ease-out',
            transform: [open ? 'translateX(0)' : 'translate(-100%)', 'none'],
            bg: ['background', 'transparent'],
            ...{ WebkitOverflowScrolling: 'touch' },
            // WebkitOverflowScrolling is not supported in our types
          }}
        />

I just wanted to mention the workaround for posterity. For this particular case, I think I have a fix.

@hasparus hasparus merged commit 9637bcd into system-ui:master May 18, 2020
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.

None yet

2 participants