Skip to content

pr05 Typescript Migration #7: migrate client components #3619

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Sep 4, 2025
Merged

pr05 Typescript Migration #7: migrate client components #3619

merged 25 commits into from
Sep 4, 2025

Conversation

clairep94
Copy link
Collaborator

@clairep94 clairep94 commented Sep 1, 2025 •
edited
Loading

pr05 Typescript Migration 7: Migrate the client/components folder

Context:

  1. git mv someComponent.jsx someComponent.tsx. If possible, commit without the no-verify flag, otherwise with no-verify.
  2. Check for all instances of useage for the common component to determine props types
  3. Add props types & unit test update
  4. Add JSDocs
  • did not refactor or add many unit tests.

Changes:

Client/components:

  • Dropdown
    • Saw that only the styled components wrapper was being used and only used once, so migrated the wrapper to Dropdown/DropdownMenu & deleted the file
  • Dropdown/DropdownMenu
  • Dropdown/MenuItem
  • Dropdown/TableDropdown
  • Menubar/Menubar
  • Menubar/MenubarItem
  • Menubar/MenubarSubmenu -- was not able to figure out the logic of this file. Got stuck on previous PR attempt and abandonned. Will take a look again on a separate PR.
  • Menubar/contexts
  • PreviewNav
  • RootPage
  • useAsModal -- deleted as this is unused

Client/common:

  • useModalClose
    • update to support generic typing
  • usePrevious
    • update to support generic typing

Other:

  • any updates to use named exports

Notes:

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@clairep94
ButtonOrLink: extend props with HTMLElements
7b61989
@clairep94
useModalClose: update to use T
ef9b265
@clairep94
usePrevious: update to use T
342d2ab
@clairep94
useAsModal: delete -- unused
067fea8
@clairep94
RootePage: update to tsx, no-verify
5f511f6
@clairep94
RootPage: update to named export and add props interface
f51af61
@clairep94
PreviewNav: migrate to tsx, no-verify
a77d064
@clairep94
PreviewNav: update to named export, add custom.d.ts for svg definitio…
4b03c97 
…n, and add props interface
@clairep94
Dropdown: Migrate DropdownWrapper to DropdownMenu and delete file due…
160a962 
… to none-use
@clairep94
DropdownMenu: update to tsx, no-verify
f017cad
@clairep94
DropdownMenu: add props interfaces, update to named export --no-verify
62cafae
@clairep94
MenuItem: update to tsx --no-verify
64bac25
@clairep94
MenuItem: add props interface and update to named export --no-verify
e80fa38
@clairep94
TableDropdown: update to tsx, no-verify
26c65c3
@clairep94
TableDropdown: add interface and update to named export, no-verify
abc09e9
@clairep94
useModalClose: add normaliser for when passedRef is a forwarded ref
43cb138
@clairep94
Menubar/contexts: update to tsx --no-verify
f9042a0
@clairep94
Menubar: update component & test to tsx, no verify
5a74f2a
@clairep94
Menubar: add interface, fix type errors, update context types and upd…
ca99e21 
…ate to named export
@clairep94
MenubarItem: migrate to tsx, no-verify
69ac8e3
@clairep94
MenubarItem: add interface, update to named export, update submenu co…
1fc953f 
…ntext
@clairep94 clairep94 changed the title Pr05/migrate client components final rebuild Pr05/migrate client components Sep 1, 2025
@clairep94 clairep94 added the pr05 Grant Projects pr05 Grant Projects label Sep 1, 2025
@clairep94 clairep94 marked this pull request as ready for review September 1, 2025 18:35
clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
.eslintrc Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/Button.stories.jsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/Button.test.tsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/Button.tsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/ButtonOrLink.test.tsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/ButtonOrLink.tsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/useModalClose.js Outdated
Copy link
Collaborator Author

@clairep94 clairep94 Sep 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/useModalClose.ts Outdated
Copy link
Collaborator Author

@clairep94 clairep94 Sep 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

updated this to use generic <T>

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/useModalClose.test.tsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/usePrevious.ts Outdated
Copy link
Collaborator Author

@clairep94 clairep94 Sep 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

updated this to use generic <T>

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, good idea!

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/usePrevious.js Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/usePrevious.test.tsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/useSyncFormTranslations.js Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/useSyncFormTranslations.test.tsx Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/common/useSyncFormTranslations.ts Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ignore, repeat from /common

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/components/Dropdown.jsx
Comment on lines -7 to -71
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;
}
}
`;
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 was the only portion of the code being used so i moved it into /DropdownMenu/DropdownMenu directly

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/components/Dropdown/DropdownMenu.tsx Outdated
Comment on lines 23 to 29
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;'}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

clairep94
clairep94 commented Sep 1, 2025
View reviewed changes
client/components/Dropdown/DropdownMenu.tsx
Comment on lines +23 to +26
right: ${(props) =>
props.align === DropdownMenuAlignment.RIGHT ? 0 : 'initial'};
left: ${(props) =>
props.align === DropdownMenuAlignment.LEFT ? 0 : 'initial'};
Copy link
Collaborator Author

@clairep94 clairep94 Sep 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

 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

khanniie
khanniie previously approved these changes Sep 2, 2025
View reviewed changes
Copy link
Collaborator

@khanniie khanniie left a comment

Choose a reason for hiding this comment

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

Looking great!! thank you so much!

client/components/Dropdown/TableDropdown.tsx
@@ -20,8 +24,10 @@ const TableDropdownIcon = () => {
);
};

const TableDropdown = styled(DropdownMenu).attrs({
align: 'right',
export interface TableDropdownProps extends DropdownMenuProps {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might be able to alias it as

export type TableDropdown = DropdownMenuProps

https://github.com/processing/p5.js-web-editor/pull/3619/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

client/components/Menubar/contexts.tsx
createMenuHandlers: (
id: string
) => {
onMouseOver: (e: React.MouseEvent) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@khanniie good point! updated

might need a re-approve as the change would dismiss the previous approval

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM!! ty!

@khanniie khanniie requested a review from raclim September 2, 2025 01:33
@clairep94 clairep94 changed the title Pr05/migrate client components pr05 Typescript Migration #7: migrate client components Sep 2, 2025
@clairep94 clairep94 mentioned this pull request Sep 3, 2025
Open
6 tasks
raclim
raclim previously approved these changes Sep 3, 2025
View reviewed changes
Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

Thanks so much for your work on this @clairep94 and @khanniie!

I really appreciate how much thought and care has went into this, and I think the level of detail in these changes/documentation will make it so much easier for incoming contributors to get onboarded!

I think overall this looks great to me—I'll merge it in after @khanniie gives it a re-review!

khanniie
khanniie previously approved these changes Sep 3, 2025
View reviewed changes
@khanniie
Copy link
Collaborator

khanniie commented Sep 3, 2025

Thanks so much for your work on this @clairep94 and @khanniie!

I really appreciate how much thought and care has went into this, and I think the level of detail in these changes/documentation will make it so much easier for incoming contributors to get onboarded!

I think overall this looks great to me—I'll merge it in after @khanniie gives it a re-review!

not sure why I can't reply directly haha but @raclim tysm!! does it make more sense to merge #3565 first? this one has a lot of repeat files from that PR, but that one is still waiting on a review from you! just wanted to flag in case it got lost~

@raclim
Copy link
Collaborator

raclim commented Sep 4, 2025

@khanniie Thanks for the reminder, sorry for missing that! Since most of the changes from that PR are here, I'll merge that one in and then this one right afterwards!

@clairep94 clairep94 dismissed stale reviews from khanniie and raclim September 4, 2025 12:52

The merge-base changed after approval.

raclim
raclim approved these changes Sep 4, 2025
View reviewed changes
Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

Sorry for dismissing the prior reviews! Since we have a history of them, I'm going to go ahead with getting this in!

@raclim raclim merged commit 7a52a44 into processing:develop Sep 4, 2025
1 check passed
@clairep94 clairep94 mentioned this pull request Sep 7, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raclim raclim raclim approved these changes

@khanniie khanniie khanniie left review comments

Assignees
No one assigned
Labels
pr05 Grant Projects pr05 Grant Projects
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clairep94 @khanniie @raclim