Updates #111
Updates #111
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
gilbarbara
force-pushed
the
updates
branch
2 times, most recently
from
6f6de06 to
f53b669
Compare
- update GH Actions workflow
- update tests
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces significant updates to improve the functionality, maintainability, and user experience of the react-floater library. The changes modernize the codebase with updated dependencies, enhanced testing patterns, and new customization features.
Key changes include:
- Added custom arrow support via a new
arrowprop for enhanced visual customization - Modernized React import patterns and hooks usage throughout the codebase
- Updated testing approach with improved test utilities and consolidated snapshot testing
- Enhanced documentation with better examples and comprehensive prop tables
Reviewed Changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/index.spec.tsx | Refactored tests with modern React imports, improved test utilities, and added custom arrow testing |
| test/snapshots/*.snap | Updated snapshots to reflect structural changes in component output |
| test/fixtures/components.tsx | Modernized React imports and component organization |
| src/types/common.ts | Added arrow prop type definition and reorganized type structure |
| src/modules/hooks.ts | Removed unused useSingleton hook |
| src/modules/helpers.ts | Removed unused React import and isValidElement helper |
| src/index.tsx | Major refactor with modern React hooks, improved effect management, and arrow prop support |
| src/components/*.tsx | Updated all components with modern React imports and arrow prop integration |
| pnpm-workspace.yaml | Added ESLint dependency hoisting configuration |
| package.json | Updated dependencies to latest versions |
| demo/src/examples/*.tsx | Enhanced examples with custom arrow demonstrations |
| README.md | Comprehensive documentation improvements with better structure and examples |
| .github/workflows/main.yml | Updated CI workflow with pnpm v10 and Node.js v22 |
Comments suppressed due to low confidence (1)
test/index.spec.tsx:30
- [nitpick] The variable name
idWrappercould be more descriptive. Consider renaming towrapperIdorwrapperTestIdto better indicate its purpose as a test identifier.
const idWrapper = `${id}-wrapper`;
| view = setup({ ...localProps, event: 'hover' }); | ||
|
|
||
| fireEvent.mouseEnter(getByDataId()); | ||
| fireEvent.mouseEnter(screen.getAllByTestId(idWrapper)[0]); |
There was a problem hiding this comment.
Using getAllByTestId(...)[0] suggests there might be multiple elements with the same test ID, which could lead to flaky tests. Consider using getByTestId instead or ensuring test IDs are unique.
| fireEvent.mouseEnter(screen.getAllByTestId(idWrapper)[0]); | |
| fireEvent.mouseEnter(screen.getByTestId(idWrapper)); |
| }, []); | ||
|
|
||
| useSingleton(() => { | ||
| // Global load event listener (singleton) |
There was a problem hiding this comment.
The global load event listener is added in every component instance but only removed in the cleanup of individual instances. This could lead to multiple listeners being attached if multiple ReactFloater components are mounted, but only the last one's cleanup will remove the listener.
src/components/Floater/Arrow.tsx
Outdated
|
|
||
| if (arrow) { | ||
| return ( | ||
| <span ref={arrowRef} style={{ color, height: length, width: spread }}> |
There was a problem hiding this comment.
[nitpick] The custom arrow styling uses height: length, width: spread but this might not match the expected dimensions for all arrow orientations. Consider using consistent dimension naming or adjusting based on placement direction.
| <span ref={arrowRef} style={{ color, height: length, width: spread }}> | |
| <span | |
| ref={arrowRef} | |
| style={{ | |
| color, | |
| height: placement.startsWith('left') || placement.startsWith('right') ? spread : length, | |
| width: placement.startsWith('left') || placement.startsWith('right') ? length : spread, | |
| }} | |
| > |
- length to size - spread to base
|
This pull request introduces several updates to improve the functionality, maintainability, and user experience of the project. Key changes include enhancements to the GitHub Actions workflow, updates to the
README.mdfor better documentation, dependency upgrades, and new customization options for the Floater component. Below is a detailed breakdown of the most important changes:Workflow and CI Updates
.github/workflows/main.ymlto usepnpmversion 10 and Node.js version 22, enabling native caching ofpnpmdependencies and simplifying setup.sonarqube-scan-actionfor improved compatibility.Documentation Enhancements
README.mdwith clearer descriptions, improved formatting, and additional examples for customization and styling. Added detailed prop tables and updated usage examples to reflect new features like custom arrows and styles. [1] [2] [3] [4]Component Customization
Floatercomponent via a newarrowprop, allowing users to provide their own SVG or ReactNode for the arrow. [1] [2]Floatercomponent's styling options by extending thestylesprop to include additional customization capabilities.Dependency Upgrades
@emotion/react,@emotion/styled,typescript, and others, to their latest versions for improved performance and security. [1] [2]demo/package.jsonto align with the latest library versions and ensure compatibility.Miscellaneous Improvements
publicHoistPatterntopnpm-workspace.yamlto hoist ESLint-related dependencies, reducing duplication and improving workspace management.Targetexample in the demo to include inline styles for better visual alignment.These updates collectively enhance the developer experience, improve the library's flexibility, and ensure compatibility with modern tooling.