pr05 Typescript Migration #7: migrate client components #3619
Conversation
…ript parser on ts files
…omponents, remove react/require-default-props rule
client/common/Button.stories.jsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/Button.test.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/Button.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/ButtonOrLink.test.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/ButtonOrLink.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/IconButton.jsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/IconButton.test.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/IconButton.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/RouterTab.test.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/RouterTab.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
There was a problem hiding this comment.
ignore, repeat from /common
client/common/useKeyDownHandlers.ts
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/useModalClose.js
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/useModalClose.ts
Outdated
There was a problem hiding this comment.
updated this to use generic <T>
client/common/useModalClose.test.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/usePrevious.ts
Outdated
There was a problem hiding this comment.
updated this to use generic <T>
client/common/usePrevious.js
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
client/common/usePrevious.test.tsx
Outdated
There was a problem hiding this comment.
ignore, repeat from /common
There was a problem hiding this comment.
ignore, repeat from /common
There was a problem hiding this comment.
ignore, repeat from /common
There was a problem hiding this comment.
ignore, repeat from /common
| export const DropdownWrapper = styled.ul` | ||
| background-color: ${prop('Modal.background')}; | ||
| border: 1px solid ${prop('Modal.border')}; | ||
| box-shadow: 0 0 18px 0 ${prop('shadowColor')}; | ||
| color: ${prop('primaryTextColor')}; | ||
|
|
||
| position: absolute; | ||
| right: ${(props) => (props.right ? 0 : 'initial')}; | ||
| left: ${(props) => (props.left ? 0 : 'initial')}; | ||
|
|
||
| ${(props) => props.align === 'right' && 'right: 0;'} | ||
| ${(props) => props.align === 'left' && 'left: 0;'} | ||
|
|
||
|
|
||
| text-align: left; | ||
| width: ${remSize(180)}; | ||
| display: flex; | ||
| flex-direction: column; | ||
| height: auto; | ||
| z-index: 2; | ||
| border-radius: ${remSize(6)}; | ||
|
|
||
| & li:first-child { | ||
| border-radius: ${remSize(5)} ${remSize(5)} 0 0; | ||
| } | ||
| & li:last-child { | ||
| border-radius: 0 0 ${remSize(5)} ${remSize(5)}; | ||
| } | ||
|
|
||
| & li:hover { | ||
| background-color: ${prop('Button.primary.hover.background')}; | ||
| color: ${prop('Button.primary.hover.foreground')}; | ||
|
|
||
| * { | ||
| color: ${prop('Button.primary.hover.foreground')}; | ||
| } | ||
| } | ||
|
|
||
| li { | ||
| height: ${remSize(36)}; | ||
| cursor: pointer; | ||
| display: flex; | ||
| align-items: center; | ||
|
|
||
| & button, | ||
| & button span, | ||
| & a { | ||
| padding: ${remSize(8)} ${remSize(16)}; | ||
| font-size: ${remSize(12)}; | ||
| } | ||
|
|
||
| * { | ||
| text-align: left; | ||
| justify-content: left; | ||
|
|
||
| color: ${prop('primaryTextColor')}; | ||
| width: 100%; | ||
| justify-content: flex-start; | ||
| } | ||
|
|
||
| & button span { | ||
| padding: 0px; | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
This was the only portion of the code being used so i moved it into /DropdownMenu/DropdownMenu directly
| right: ${(props) => | ||
| props.align === DropdownMenuAlignment.RIGHT ? 0 : 'initial'}; | ||
| left: ${(props) => | ||
| props.align === DropdownMenuAlignment.LEFT ? 0 : 'initial'}; | ||
|
|
||
| ${(props) => props.align === DropdownMenuAlignment.RIGHT && 'right: 0;'} | ||
| ${(props) => props.align === DropdownMenuAlignment.LEFT && 'left: 0;'} |
There was a problem hiding this comment.
right: ${(props) => (props.right ? 0 : 'initial')};
left: ${(props) => (props.left ? 0 : 'initial')};
${(props) => props.align === 'right' && 'right: 0;'}
${(props) => props.align === 'left' && 'left: 0;'}
this is the previous code from /Dropdown for reference
Other parts of the DropdownWrapper are unchanged
| right: ${(props) => | ||
| props.align === DropdownMenuAlignment.RIGHT ? 0 : 'initial'}; | ||
| left: ${(props) => | ||
| props.align === DropdownMenuAlignment.LEFT ? 0 : 'initial'}; |
There was a problem hiding this comment.
right: ${(props) => (props.right ? 0 : 'initial')};
left: ${(props) => (props.left ? 0 : 'initial')};
${(props) => props.align === 'right' && 'right: 0;'}
${(props) => props.align === 'left' && 'left: 0;'}
this is the previous code from /Dropdown for reference
I think this is a typo where props.right and props.left would never exist, as we use props.align, so we would have right:initial and left:initial, then update to right: 0 or left: 0 based on props.align
So I tried to simplify
Other parts of the DropdownWrapper are unchanged
| @@ -20,8 +24,10 @@ const TableDropdownIcon = () => { | |||
| ); | |||
| }; | |||
|
|
|||
| const TableDropdown = styled(DropdownMenu).attrs({ | |||
| align: 'right', | |||
| export interface TableDropdownProps extends DropdownMenuProps {} | |||
There was a problem hiding this comment.
you might be able to alias it as
export type TableDropdown = DropdownMenuProps
https://github.com/processing/p5.js-web-editor/pull/3619/files
There was a problem hiding this comment.
Ah I thought maybe we could do this as an extended interface so we can extend with TableDropdown-specific props in the future, but happy to change to a type if preferred! I might have overthought this haha maybe TableDropdown is just a styled DropdownMenu
| createMenuHandlers: ( | ||
| id: string | ||
| ) => { | ||
| onMouseOver: (e: React.MouseEvent) => void; |
There was a problem hiding this comment.
I'm not strongly opinionated about this so feel free to just acknowledge and ignore, but I noticed that the logic changes a bit in this file where the factory methods (e.g. createMenuHandlers) previously would return an empty dict {} but now return dicts where the value is a function that doesn't do anything. I think if there's logic further down the line that checks somehting like menuHandlers.onMouseOver, the return would be different. in the past where it would be undefined now it would return () => {}. if you added the null functions to avoid type errors, we could just make the handlers optional? just wanted to flag it in case
There was a problem hiding this comment.
@khanniie good point! updated
might need a re-approve as the change would dismiss the previous approval
clairep94
changed the title
pr05 Typescript Migration 7: Migrate the client/components folder
Context:
Rebuild of Pr05/migrate client components rebuild #3616 after getting stuck on MenubarSubmenu
Built on top of pr05 Typescript Migration #5: Migrate client/common folder #3565, should be reviewed after this PR is merged.
Migrate as much of the client/components folder with the following steps:
git mv someComponent.jsx someComponent.tsx. If possible, commit without theno-verifyflag, otherwise withno-verify.Changes:
Client/components:
Client/common:
Other:
Notes:
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123