-
Notifications
You must be signed in to change notification settings - Fork 357
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
chore(DashboardWrapper + DashboardHeader) convert to TS #9618
chore(DashboardWrapper + DashboardHeader) convert to TS #9618
Conversation
Preview: https://patternfly-react-pr-9618.surge.sh A11y report: https://patternfly-react-pr-9618-a11y.surge.sh |
f90c511
to
434ac28
Compare
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.
Fixed all module resolution errors and this is working and building as expected locally - currently investigating rollup issue in build:umd
step in CI.
I've moved the DashboardWrapper
and DashboardHeader
components into react-core
since it makes more sense to import them into files with the other react-core imports. This is going to need some build/infra related updates.
1073223
to
93bae2b
Compare
chore(DashboardWrapper) convert to TS move to utils and update imports fix md line fix build errors update columnmanagement dashboardwrapper import testing move files out of utils change to absolute imports change to relative imports for each component update page import add rollup plugin-typescript rename rollup.config.js to rollup.config.mjs revert build changes and remove imports from top of md files revert yarn.lock, revert to index.d.ts, update jumplinks import
93bae2b
to
bf2ce5c
Compare
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.
Update: Added rollup/plugin-svg
which was missing from configs and causing svg imports to break the build. This was breaking jest tests as well which needed their own svg transformer jest-transform-stub
.
These updates to the build here should also close patternfly/patternfly-org#3757 in org.
Note: merging this should unblock a lot of the remaining work/PRs in the demo conversion epic , several of which depended on the Dashboard components getting converted to TS
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.
Looks like there's a few Table demos still importing the old DashboardWrapper from the demos/examples
directory (Empty, Loading, and Error in the "Empty states" section of examples). Otherwise looks good. Only thing is if we'd want to have the Dashboard components able to be imported by consumers; there'd probably be not much use-case for them to import it in the first place, but just wondering if the files should remain in the demos directory, or just not exported in our index file.
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.
Thank you @thatblindgeye - good point. At the moment our build-configs don't seem to support TSX exports from demos/examples
, so I'm investigating how we can make the necessary updates to move these Dashboard components back to the demos
folder.
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.
LGTM
What:
When yarn start is run there are a few module resolution changes I am not able to solve.