-
Notifications
You must be signed in to change notification settings - Fork 13
LSPAY-33449: React18 typings #190
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
base: next
Are you sure you want to change the base?
Conversation
2312531 to
e723c51
Compare
- Typings, - Updating gh actions, depracated actions/cache@v1 - Allow prerelease on branch LSPAY-33449
b9d67ce to
8f2da7f
Compare
1669c81 to
0d8b904
Compare
- @lightspeed/[email protected] - @lightspeed/[email protected]
nwallace534
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.
Understand this has been in use already. I skim reviewed and added a few questions but no blockers found and all code has been glanced at.
Sure glad I don't use snapshot testing anymore, it doesn't add anything but lines to review in PRs.
| // TODO: use prettier.resolveConfigFile instead... | ||
| prettierConfig.parser = 'babel'; | ||
|
|
||
| // Function to sanitize SVG attributes for JSX compatibility |
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.
Discussed via slack. AI generated script to fill in for lib no longer compatible.
| import * as React from 'react'; | ||
| import { storiesOf } from '@storybook/react'; | ||
| import { withReadme } from 'storybook-readme'; | ||
| import Readme from './README.md'; |
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.
Is this local README.md still in the repo or referenced elsewhere? If not I'd recommend removing
| import { Tooltip, TooltipPlacement } from './Tooltip'; | ||
| import { Input } from '../Input'; | ||
| import { ExampleBox } from '../../../../.storybook/components/ExampleBox'; | ||
| import Readme from './README.md'; |
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.
Mentioned elsewhere, does this README still get uesd?
| "@styled-system/theme-get": "5.0.16", | ||
| "styled-system": "^5.0.16" | ||
| }, | ||
| "peerDependencies": { |
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.
Not a comment on your PR but I do wonder why there is a handlebars file with a package.json like what could it possibly be adding to a project.
| "command": { | ||
| "publish": { | ||
| "allowBranch": ["master", "next"], | ||
| "allowBranch": ["master", "next", "LSPAY-33449"], |
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.
will this hardcoded branch be removed soon?
| "prop-types": "^15.6.0", | ||
| "react": "^16.8.6", | ||
| "react-dom": "^16.8.6", | ||
| "react": "^18.3.1", |
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.
The reason for the whole PR!
JIRA Ticket: LSPAY-33449
Description
Adding React18 typings
How to test?
yarn devChecklist