feat: add Brush mark #12
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gka
force-pushed
the
feat/brush
branch
2 times, most recently
from
300b7ce to
f914ac9
Compare
✅ Deploy Preview for svelteplot ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds interactive brushing functionality to SveltePlot by introducing the Brush, BrushX, and BrushY components along with comprehensive tests, updated documentation, and support for additional scale types and pointer events.
- Added Brush components with support for dragging and dimension-limited brushing.
- Updated type definitions to use the csstype package and improved event handling.
- Updated tests, documentation, and sidebar configuration to integrate the new brushing feature.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tests/brush.test.svelte | Added component-based tests for the Brush component. |
| src/tests/brush.svelte.test.ts | Added unit tests to validate brushing reactions and interactions. |
| src/routes/marks/brush/+page.ts | Updated page load to include additional datasets. |
| src/lib/types.ts | Updated CSS property type definitions using csstype. |
| src/lib/marks/helpers/events.ts | Enhanced event handling with extended pointer events. |
| src/lib/marks/Rect.svelte | Refactored class name default to improve consistency. |
| src/lib/marks/Frame.svelte | Refined type definitions and removed redundant style blocks. |
| src/lib/marks/Brush*.svelte | New Brush components to enable rectangle-based selections. |
| src/lib/marks/Brush.svelte | Core brushing functionality implemented with interaction logic. |
| src/lib/Plot.svelte | Wrapped plot rendering in an error boundary for graceful errors. |
| src/app.scss, package.json, config/sidebar.ts, CHANGELOG.md | Updated styling, dependencies, sidebar configuration, and changelog. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces Brush, BrushX, and BrushY components to add brushing support to SveltePlot with comprehensive tests, documentation, and updated API behavior. Key changes include:
- Implementation of interactive brushing via a new Brush component (and its one-dimensional variants).
- Comprehensive unit tests and support for various scale types (e.g., time, log) and pointer events.
- Refactoring of related helper functions and update of type definitions to integrate with the csstype package.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/brush.test.svelte | New Svelte test component for verifying brush behavior. |
| src/tests/brush.svelte.test.ts | Unit tests for brush interactivity including pointer events and drag tests. |
| src/routes/marks/brush/+page.ts | Updated page loader now includes an extra dataset for demo purposes. |
| src/lib/types.ts | Updated type definitions using csstype for CSS properties. |
| src/lib/marks/helpers/events.ts | Added pointer event support and removed extraneous logging. |
| src/lib/marks/Rect.svelte | Updated default class name usage to improve consistency. |
| src/lib/marks/Frame.svelte | Adjusted props defaults and removed unused style block. |
| src/lib/marks/Cell.svelte | Removed obsolete styles. |
| src/lib/marks/Brush*, Brush.svelte | New Brush components with complex resize and drag logic for brushing. |
| src/lib/Plot.svelte | Enhanced error handling in plot rendering with svelte:boundary. |
| package.json, config/sidebar.ts, CHANGELOG.md | Ancillary changes for dependency updates and documentation highlights. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces interactive brushing support for SveltePlot by adding new Brush, BrushX, and BrushY components along with comprehensive tests, updated documentation, and related style and dependency changes.
- Implements a Brush component that allows users to create, move, and resize brush selections within the plot.
- Enhances event handling for pointer interactions and updates CSS type definitions using the csstype package.
- Updates demo pages, sidebar configuration, and documentation to showcase the new brushing features.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tests/brush.test.svelte | Added a basic test for rendering the Brush component. |
| src/tests/brush.svelte.test.ts | Added tests to simulate user drag events and brush state changes. |
| src/routes/marks/brush/+page.ts | Updated dataset loading for the brush demo. |
| src/lib/types.ts | Updated CSS property types to use csstype instead of custom types. |
| src/lib/marks/helpers/events.ts | Refined pointer event handlers and removed debugging logs. |
| src/lib/marks/Rect.svelte | Adjusted class name handling for rectangle marks. |
| src/lib/marks/Frame.svelte | Tweaked prop defaults and removed unused style sections. |
| src/lib/marks/Brush*.svelte | Added new Brush components with support for dragging, resizing, etc. |
| src/lib/marks/Brush.svelte | Introduces the primary brushing logic with pointer event handling. |
| src/lib/Plot.svelte | Wrapped plot content in error boundaries and improved error display. |
| src/app.scss | Updated code styling for pre elements. |
| package.json | Added new dependencies for user-event and csstype. |
| config/sidebar.ts | Added a sidebar entry for the Brush mark. |
| CHANGELOG.md | Updated changelog with new brushing features. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces brush marks and their one-dimensional variants (BrushX and BrushY) to add brushing support to SveltePlot, along with corresponding tests, documentation updates, and supporting style and type adjustments.
- Adds a new Brush component with support for dragging to create a rectangular selection.
- Introduces BrushX and BrushY components that limit brushing to a single dimension.
- Updates tests, page data loading, and documentation to reflect the new brushing functionality.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/brush.test.svelte | Adds Svelte tests verifying basic brush functionality. |
| src/tests/brush.svelte.test.ts | Includes unit tests simulating user drag events for the brush. |
| src/routes/marks/brush/+page.ts | Updates dataset loading to support the brush demo. |
| src/lib/types.ts | Replaces custom string unions with csstype types. |
| src/lib/marks/helpers/events.ts | Enhances event helpers with pointer event support and cleans logging. |
| src/lib/marks/Rect.svelte | Adjusts class name defaults for consistency with the new brush mark. |
| src/lib/marks/Frame.svelte | Modifies prop defaults to ensure transparent interaction layers. |
| src/lib/marks/Cell.svelte | Removes unused inline styles. |
| src/lib/marks/Brush*, Brush.svelte | Introduces the main Brush component and its one-dimensional variants. |
| src/lib/Plot.svelte | Adds error boundary and displays errors in the brush demo page. |
| Others (app.scss, package.json, config sidebar) | Minor styling, dependency, and navigation updates for the brush. |
| CHANGELOG.md | Documents the brush component additions and related improvements. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/lib/marks/Brush.svelte:337
- [nitpick] The swapIfNeeded function manipulates the action string in a non-obvious way when flipping coordinates; adding inline comments to explain the transformation would help future maintainers understand the intent.
return [v2, v1, `${action.split('-')[0].replace(swapDir1, 'X').replace(swapDir2, swapDir1).replace('X', swapDir2)}-resize` as ActionType];
src/lib/marks/Brush.svelte
Outdated
Comment on lines
248
to
265
| if (action === 'e-resize' || action === 'ne-resize' || action === 'se-resize') { | ||
| px = constrain(px, [xRange[0] - pxBrush.x2, xRange[1] - pxBrush.x2]); | ||
| } else if ( | ||
| action === 'w-resize' || | ||
| action === 'nw-resize' || | ||
| action === 'sw-resize' | ||
| ) { | ||
| px = constrain(px, [xRange[0] - pxBrush.x1, xRange[1] - pxBrush.x1]); | ||
| } | ||
| if (action === 'n-resize' || action === 'ne-resize' || action === 'nw-resize') { | ||
| py = constrain(py, [yRange[0] - pxBrush.y2, yRange[1] - pxBrush.y2]); | ||
| } else if ( | ||
| action === 's-resize' || | ||
| action === 'se-resize' || | ||
| action === 'sw-resize' | ||
| ) { | ||
| py = constrain(py, [yRange[0] - pxBrush.y1, yRange[1] - pxBrush.y1]); | ||
| } |
There was a problem hiding this comment.
[nitpick] The conditional blocks for handling resize actions are quite complex; refactoring the resizing logic into smaller helper functions could simplify the overall code structure and improve maintainability.
Suggested change
| if (action === 'e-resize' || action === 'ne-resize' || action === 'se-resize') { | |
| px = constrain(px, [xRange[0] - pxBrush.x2, xRange[1] - pxBrush.x2]); | |
| } else if ( | |
| action === 'w-resize' || | |
| action === 'nw-resize' || | |
| action === 'sw-resize' | |
| ) { | |
| px = constrain(px, [xRange[0] - pxBrush.x1, xRange[1] - pxBrush.x1]); | |
| } | |
| if (action === 'n-resize' || action === 'ne-resize' || action === 'nw-resize') { | |
| py = constrain(py, [yRange[0] - pxBrush.y2, yRange[1] - pxBrush.y2]); | |
| } else if ( | |
| action === 's-resize' || | |
| action === 'se-resize' || | |
| action === 'sw-resize' | |
| ) { | |
| py = constrain(py, [yRange[0] - pxBrush.y1, yRange[1] - pxBrush.y1]); | |
| } | |
| px = constrain(px, getPxConstraint(action, xRange, pxBrush)); | |
| py = constrain(py, getPyConstraint(action, yRange, pxBrush)); | |
| // limit resizing | |
| px = constrain(px, getPxConstraint(action, xRange, pxBrush)); | |
| py = constrain(py, getPyConstraint(action, yRange, pxBrush)); |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces brushing support to SveltePlot by adding Brush, BrushX, and BrushY components along with their tests, documentation, and necessary adjustments to underlying types and event handling.
- Added new brush components and tests
- Updated event handling and type definitions to support pointer and touch events
- Expanded docs, changelog, and sidebar configuration to include the new components
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/brush.test.svelte | Added Svelte test for basic Brush component usage |
| src/tests/brush.svelte.test.ts | Added unit tests covering brush interaction |
| src/routes/marks/brush/+page.ts | Updated dataset loading for brush demo |
| src/lib/types.ts | Updated type definitions for CSS properties |
| src/lib/marks/helpers/events.ts | Extended event typings and refactored helper logic |
| src/lib/marks/Rect.svelte | Modified default class naming and inset fallback |
| src/lib/marks/Frame.svelte | Adjusted props and default class initialization |
| src/lib/marks/Cell.svelte | Removed obsolete inline styles |
| src/lib/marks/Brush*.svelte | Introduced Brush, BrushX, and BrushY components |
| src/lib/marks/Brush.svelte | Implemented core Brush functionality including drag handling and coordinate swapping |
| src/lib/Plot.svelte | Added error boundary with styled error messages |
| src/app.scss | Updated code font styles for pre elements |
| package.json | Added testing and CSS type dependencies |
| config/sidebar.ts | Updated sidebar navigation for the new Brush demo |
| CHANGELOG.md | Documented new Brush functionalities |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Preview: https://feat-brush--svelteplot.netlify.app/marks/brush
I was wondering how SveltePlot could add support for brushing. Maybe it makes sense to add helper components similar to Pointer, e.g. Brush, BrushX and BrushY. Here are a few demos for how it could work:
This is what I came up with so far:
Todos: