fix: constants refactor#25
fix: constants refactor#25p55d2k wants to merge 4 commits intosatnam72:mainfrom p55d2k:fix/constants-refactor
Conversation
WalkthroughCentralizes UI and behavior literals into Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(46, 125, 50, 0.08)
participant User
participant Allonsh
participant DOM
participant Dropzone
end
User->>Allonsh: start drag (mousedown)
Allonsh->>DOM: create ghost (position: CSS_POSITIONS.ABSOLUTE, z-index: Z_INDEX.GHOST, opacity: OPACITY.GHOST, pointer-events: POINTER_EVENTS.NONE)
Allonsh->>DOM: apply dragging visuals (cursor: CSS_CURSORS.GRABBING, z-index: Z_INDEX.DRAGGING)
Allonsh->>Dropzone: dispatch EVENTS.DRAG_ENTER / EVENTS.DRAG_LEAVE as pointer moves
Dropzone-->>Allonsh: respond (toggle CSS_CLASSES.HIGHLIGHT)
User->>Allonsh: drop (mouseup)
Allonsh->>DOM: remove ghost, restore visuals (pointer-events: POINTER_EVENTS.AUTO, opacity: OPACITY.FULL)
Allonsh->>DOM: dispatch EVENTS.DROP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/allonsh.js (3)
16-27: Persist the selectors on the instance; needed for safe update().update() re-queries using parameters, but you don’t persist draggableSelector/dropzoneSelector on the instance. If update() is called without those fields, it currently queries ".undefined".
Apply this diff:
constructor(options = {}) { const { draggableSelector, dropzoneSelector = null, playAreaSelector = null, restrictToDropzones = false, enableStacking = false, stackDirection = DEFAULTS.STACK_DIRECTION, stackSpacing = DEFAULTS.STACK_SPACING, useGhostEffect = false, } = options; + // Persist selectors for later updates + this.draggableSelector = draggableSelector; + this.dropzoneSelector = dropzoneSelector;
97-108: update(): avoid querying with undefined selectors; preserve prior values.When draggableSelector is omitted, you end up querying with ".undefined". Use fallbacks to the persisted selectors and update them only when explicitly provided.
Apply this diff:
update(newOptions = {}) { const { - draggableSelector, - dropzoneSelector, - playAreaSelector, + draggableSelector, + dropzoneSelector, + playAreaSelector, restrictToDropzones, enableStacking, stackDirection, stackSpacing, useGhostEffect, } = newOptions; + // Compute effective selectors + const effectiveDraggableSel = draggableSelector ?? this.draggableSelector; + const effectiveDropzoneSel = dropzoneSelector ?? this.dropzoneSelector; + if (draggableSelector !== undefined) this.draggableSelector = draggableSelector; + if (dropzoneSelector !== undefined) this.dropzoneSelector = dropzoneSelector; @@ if (playAreaSelector) { const newPlayArea = document.querySelector(`.${playAreaSelector}`); if (newPlayArea) { this.playAreaElement = newPlayArea; - this.playAreaElement.style.position = 'relative'; + this.playAreaElement.style.position = CSS_POSITIONS.RELATIVE; } else { console.warn( `Allonsh Warning: Play area element with class '${playAreaSelector}' not found.` ); } } this._unbindEventListeners(); - this.draggableElements = this.playAreaElement.querySelectorAll( - `.${draggableSelector}` - ); + this.draggableElements = this.playAreaElement.querySelectorAll( + `.${effectiveDraggableSel}` + ); @@ - if (dropzoneSelector) { + if (effectiveDropzoneSel) { this.dropzoneElements = this.playAreaElement.querySelectorAll( - `.${dropzoneSelector}` + `.${effectiveDropzoneSel}` ); this._dropzoneSet = new Set(this.dropzoneElements); }Also applies to: 116-126, 129-146
176-183: Fix listener leak by using pre-bound handlers
The grep results confirm thatthis._onMouseDown.bind(this)andthis._onTouchStart.bind(this)are still being attached, so_unbindEventListeners()cannot remove them. Please replace these bound-in-place closures with the pre-bound handler references.Affected location:
- src/allonsh.js, around lines 179–180
Apply this diff:
--- a/src/allonsh.js +++ b/src/allonsh.js @@ 176,7 +176,7 @@ this.draggableElements.forEach((element) => { try { - element.style.cursor = CSS_CURSORS.GRAB; - element.addEventListener('mousedown', this._onMouseDown.bind(this)); - element.addEventListener('touchstart', this._onTouchStart.bind(this), { - passive: false, - }); + element.style.cursor = CSS_CURSORS.GRAB; + element.addEventListener('mousedown', this._boundMouseDown); + element.addEventListener('touchstart', this._boundTouchStart, { + passive: false, + }); } catch (err) { console.error( 'Allonsh Error attaching event listeners to draggable element:', err ); } });This ensures that
_unbindEventListeners()can successfully remove the handlers.
🧹 Nitpick comments (19)
demo/css/styles.css (1)
60-60: Consider promoting z-indexes to CSS custom properties to avoid drift with JS constants.You’ve added good breadcrumbs in comments. To lock alignment with src/constants.js and make future tweaks trivial, introduce z-index tokens in :root and reference them here.
Apply this diff as a starting point:
:root { + /* z-index tokens (mirrors JS Z_INDEX) */ + --z-header: 10; + --z-theme-toggle-btn: 9; + --z-control-panel: 9; + --z-freemode-title: 5; + /* Note: demo-only button uses 1000 to sit above header */ + --z-toggle-button: 1000; } .demo-header { position: fixed; - z-index: 10; /* This could be updated to use CSS custom properties if needed */ + z-index: var(--z-header); ... } .toggle-button { ... - z-index: 1000; /* This could be updated to use CSS custom properties if needed */ + z-index: var(--z-toggle-button); ... } .freemode-title { ... - z-index: 5; /* This could be updated to use CSS custom properties if needed */ + z-index: var(--z-freemode-title); ... } .theme-toggle-btn { ... - z-index: 9; /* This could be updated to use CSS custom properties if needed */ + z-index: var(--z-theme-toggle-btn); ... } @media (max-width: 768px) { .demo-control-panel { ... - z-index: 9; /* This could be updated to use CSS custom properties if needed */ + z-index: var(--z-control-panel); ... } }Also verify that dragged elements (Z_INDEX.DRAGGING = 1000) don’t unintentionally occlude .toggle-button (also 1000). If needed, bump one side slightly to guarantee expected stacking.
Also applies to: 229-229, 290-290, 430-430, 449-449
src/constants.js (3)
6-14: Align Z_INDEX with demo CSS and reserve room for “toggle-button”.Constants look good and map well to runtime needs. Given the demo CSS has both .theme-toggle-btn (9) and .toggle-button (1000), consider documenting/adding a TOGGLE_BUTTON key to reflect the demo-only element and avoid confusion.
Apply this diff:
export const Z_INDEX = { GHOST: 999, DRAGGING: 1000, DROPPED: 9999, HEADER: 10, - THEME_TOGGLE: 9, + THEME_TOGGLE: 9, // matches .theme-toggle-btn + TOGGLE_BUTTON: 1000, // matches .toggle-button (demo-only) CONTROL_PANEL: 5, FREEMODE_TITLE: 5 };
16-22: Separate “defaults” from “allowed values” to avoid duplication.DEFAULTS currently carries both the default stack direction and the allowed values (STACK_DIRECTION_VERTICAL/HORIZONTAL). This couples concerns and invites drift.
Refactor into an enum for allowed values and keep DEFAULTS for actual defaults:
-export const DEFAULTS = { - STACK_SPACING: 5, - STACK_SPACING_DEMO: 10, - STACK_DIRECTION: 'horizontal', - STACK_DIRECTION_VERTICAL: 'vertical', - STACK_DIRECTION_HORIZONTAL: 'horizontal' -}; +export const STACK_DIRECTIONS = Object.freeze({ + VERTICAL: 'vertical', + HORIZONTAL: 'horizontal', +}); + +export const DEFAULTS = Object.freeze({ + STACK_SPACING: 5, + STACK_SPACING_DEMO: 10, + STACK_DIRECTION: STACK_DIRECTIONS.HORIZONTAL, +});Then call sites can compare against STACK_DIRECTIONS.VERTICAL/HORIZONTAL instead of DEFAULTS.*.
Also applies to: 53-56
6-14: Freeze exported objects to prevent accidental mutation at runtime.These are configuration constants; locking them down reduces accidental overrides.
Apply this diff:
-export const Z_INDEX = { +export const Z_INDEX = Object.freeze({ GHOST: 999, DRAGGING: 1000, DROPPED: 9999, HEADER: 10, THEME_TOGGLE: 9, CONTROL_PANEL: 5, FREEMODE_TITLE: 5 -}; +}); -export const DEFAULTS = { +export const DEFAULTS = Object.freeze({ STACK_SPACING: 5, STACK_SPACING_DEMO: 10, STACK_DIRECTION: 'horizontal', STACK_DIRECTION_VERTICAL: 'vertical', STACK_DIRECTION_HORIZONTAL: 'horizontal' -}; +}); -export const CSS_CLASSES = { +export const CSS_CLASSES = Object.freeze({ DROPZONE: 'allonsh-dropzone', HIGHLIGHT: 'allonsh-highlight', DRAGGABLE: 'allonsh-draggable', RESTRICTED: 'restricted' -}; +}); -export const CSS_POSITIONS = { +export const CSS_POSITIONS = Object.freeze({ RELATIVE: 'relative', ABSOLUTE: 'absolute' -}; +}); -export const CSS_CURSORS = { +export const CSS_CURSORS = Object.freeze({ GRAB: 'grab', GRABBING: 'grabbing' -}; +}); -export const OPACITY = { +export const OPACITY = Object.freeze({ GHOST: '0.3', FULL: '1' -}; +}); -export const EVENTS = { +export const EVENTS = Object.freeze({ DRAG_START: 'allonsh-dragstart', DROP: 'allonsh-drop', DRAG_ENTER: 'allonsh-dragenter', DRAG_LEAVE: 'allonsh-dragleave' -}; +}); -export const FLEX_DIRECTIONS = { +export const FLEX_DIRECTIONS = Object.freeze({ ROW: 'row', COLUMN: 'column' -}; +}); -export const DISPLAY_MODES = { +export const DISPLAY_MODES = Object.freeze({ FLEX: 'flex', NONE: 'none' -}; +}); -export const POINTER_EVENTS = { +export const POINTER_EVENTS = Object.freeze({ NONE: 'none', AUTO: 'auto' -}; +});Also applies to: 16-22, 24-29, 31-34, 36-39, 41-44, 46-51, 53-56, 58-58, 60-63, 65-68
test/allonsh.test.js (1)
53-57: Optional: add a regression test for update() without passing selectors.Given the Allonsh refactor, calling update() without draggableSelector/dropzoneSelector should preserve existing selectors. Consider adding a spec that calls allonsh.update({ stackSpacing: 7 }) and verifies no errors and selectors remain effective.
I can draft this test if helpful.
demo/js/script.js (1)
36-36: Use constants for stack direction strings to prevent typos and ease future changes.You already import DEFAULTS; leverage DEFAULTS.STACK_DIRECTION_HORIZONTAL/DEFAULTS.STACK_DIRECTION_VERTICAL (or STACK_DIRECTIONS if introduced) for the demo UI state instead of raw strings.
Apply this diff:
-let currentStackDirection = 'horizontal'; +let currentStackDirection = DEFAULTS.STACK_DIRECTION_HORIZONTAL; ... - currentStackDirection = 'horizontal'; + currentStackDirection = DEFAULTS.STACK_DIRECTION_HORIZONTAL; ... - if (currentStackDirection !== 'vertical') { - currentStackDirection = 'vertical'; + if (currentStackDirection !== DEFAULTS.STACK_DIRECTION_VERTICAL) { + currentStackDirection = DEFAULTS.STACK_DIRECTION_VERTICAL; } ... - if (currentStackDirection !== 'horizontal') { - currentStackDirection = 'horizontal'; + if (currentStackDirection !== DEFAULTS.STACK_DIRECTION_HORIZONTAL) { + currentStackDirection = DEFAULTS.STACK_DIRECTION_HORIZONTAL; }Also applies to: 148-149, 156-170
src/allonsh.js (3)
312-352: Remove duplicate cursor assignment in _startDrag.You set GRABBING twice (Lines 312 and 350). Keep one.
Apply this diff:
- this.currentDraggedElement = event.currentTarget; - this.currentDraggedElement.style.cursor = CSS_CURSORS.GRABBING; + this.currentDraggedElement = event.currentTarget; @@ - this.currentDraggedElement.style.cursor = CSS_CURSORS.GRABBING; this.currentDraggedElement.style.zIndex = Z_INDEX.DRAGGING;
702-711: setDropzones(): also add internal class and stacking styles for new zones.Newly selected dropzones don’t receive the internal CSS class nor stacking styles unless reinitialized.
Apply this diff:
setDropzones(selector) { try { this.dropzoneElements = this.playAreaElement.querySelectorAll( `.${selector}` ); this._dropzoneSet = new Set(this.dropzoneElements); + // Apply internal class + stacking to new zones + this.dropzoneElements.forEach((dz) => { + dz.classList.add(CSS_CLASSES.DROPZONE); + if (this.enableStacking) this._applyStackingStyles(dz); + }); } catch (err) { console.error('Allonsh Error setting dropzones:', err); } }
169-170: Minor consistency: prefer CSS_POSITIONS.RELATIVE instead of string literal.One spot in constructor and one in update() still hardcode 'relative'.
Apply this diff:
- this.playAreaElement.style.position = 'relative'; + this.playAreaElement.style.position = CSS_POSITIONS.RELATIVE;Also applies to: 120-121
CONSTANTS_REFACTOR_SUMMARY.md (10)
8-19: Add missing constant group and tighten the list.The overview omits the “FLEX_WRAP” group that’s referenced later. Add it here for completeness and consistency.
Apply this diff to include the missing bullet:
### 1. New Constants File: `src/constants.js` - **Z_INDEX**: Centralized z-index values (GHOST: 999, DRAGGING: 1000, DROPPED: 9999, etc.) - **DEFAULTS**: Default configuration values (stack spacing, direction) - **CSS_CLASSES**: Consistent CSS class names - **CSS_POSITIONS**: Position values (relative, absolute) - **CSS_CURSORS**: Cursor values (grab, grabbing) - **OPACITY**: Opacity values (ghost: 0.3, full: 1) - **EVENTS**: Custom event names - **FLEX_DIRECTIONS**: Flexbox direction values + - **FLEX_WRAP**: Flex wrapping values - **DISPLAY_MODES**: Display property values - **POINTER_EVENTS**: Pointer-events values
23-56: Fix markdownlint: remove trailing colons in headings.Headings ending with “:” violate MD026. Drop the trailing punctuation on these “####” headings.
Apply this diff:
-#### Z-Index Values: +#### Z-Index Values -#### Default Values: +#### Default Values -#### CSS Class Names: +#### CSS Class Names -#### CSS Properties: +#### CSS Properties -#### Opacity Values: +#### Opacity Values -#### Pointer Events: +#### Pointer Events -#### Event Names: +#### Event Names
29-31: Clarify “horizontal” default vs flex direction mapping.You map 'horizontal' → DEFAULTS.STACK_DIRECTION while elsewhere row/column come from FLEX_DIRECTIONS. Briefly note how 'horizontal' corresponds to
FLEX_DIRECTIONS.ROWto avoid ambiguity for readers.Apply this diff to add a clarifying note:
- 'horizontal' → `DEFAULTS.STACK_DIRECTION` - `5` → `DEFAULTS.STACK_SPACING` +Note: `DEFAULTS.STACK_DIRECTION = 'horizontal'` corresponds to `FLEX_DIRECTIONS.ROW` and `'vertical'` corresponds to `FLEX_DIRECTIONS.COLUMN`.
38-47: Disambiguate FLEX_WRAP usage.The line “'wrap' → FLEX_WRAP” reads like a bare string-to-object mapping. If
FLEX_WRAPis a group, preferFLEX_WRAP.WRAP(or consolidate under aFLEX.WRAP) for symmetry with other groups.Apply this doc tweak:
-- 'wrap' → `FLEX_WRAP` +- 'wrap' → `FLEX_WRAP.WRAP`If the code currently exports a single
FLEX_WRAP = 'wrap', consider refactoring to an object for future-proofing:
- Keep:
export const FLEX_WRAP = { WRAP: 'wrap', NOWRAP: 'nowrap' }- Or fold into
FLEX = { DIRECTION: {...}, WRAP: {...} }
48-51: Use numeric literals for opacity in the doc (matches typical constants).Quoting numbers suggests strings. Prefer numeric literals to reflect intent, even if
style.opacityaccepts strings.Apply this diff:
-- '0.3' → `OPACITY.GHOST` -- '1' → `OPACITY.FULL` +- 0.3 → `OPACITY.GHOST` +- 1 → `OPACITY.FULL`
62-65: Avoid exporting demo-only defaults from the library.
DEFAULTS.STACK_SPACING_DEMOsounds demo-specific. Prefer keeping demo defaults in demo code to avoid library surface creep, or rename to a genericSTACK_SPACING_UIif it’s broadly useful.Would you like me to propose a small patch moving the demo default into
demo/js/script.jsand keepingDEFAULTSminimal?
70-73: Quick win: show example CSS custom properties (future-proofing).Since you mention future migration, a short snippet helps readers visualize the target state.
Proposed addition (no code changes implied):
+Example (future): +```css +:root { + --allonsh-z-ghost: 999; + --allonsh-z-dragging: 1000; + --allonsh-z-dropped: 9999; +} +.allonsh-ghost { z-index: var(--allonsh-z-ghost); } +```
76-82: Qualify the “Type Safety” benefit.Type-safety is realized when
constants.jsships JSDoc typedefs or.d.ts. Consider adding a note to that effect.Apply this wording tweak:
-5. **Type Safety**: Better IDE support and error detection +5. **Type Safety**: Better IDE support and error detection (add JSDoc typedefs or .d.ts to maximize benefits)
32-37: CSS class namespacing: confirmrestrictedis intentionally unprefixed.Most classes use the
allonsh-prefix, butrestrictedis not. If intentional (shared utility class), note it; otherwise, considerCSS_CLASSES.RESTRICTED = 'allonsh-restricted'to avoid collisions.If you want to align it, update the doc line and class usage accordingly. I can generate a follow-up patch if desired.
56-61: Document prefix convention and confirm no magic stringsVerified via the provided script that there are no remaining hardcoded
allonsh-…event names insrc,test, ordemocode outside ofsrc/constants.js. To complete the documentation, please add the following line to CONSTANTS_REFACTOR_SUMMARY.md under “#### Event Names:”:- 'allonsh-dragleave' → `EVENTS.DRAG_LEAVE` + Convention: All custom events use the `allonsh-` prefix (e.g., `allonsh-dragstart`, `allonsh-drop`).This succinctly describes the
allonsh-prefix convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CONSTANTS_REFACTOR_SUMMARY.md(1 hunks)demo/css/styles.css(5 hunks)demo/js/script.js(2 hunks)src/allonsh.js(20 hunks)src/constants.js(1 hunks)test/allonsh.test.js(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CONSTANTS_REFACTOR_SUMMARY.md
[grammar] ~3-~3: There might be a mistake here.
Context: ... Constants Refactor Summary ## Overview This document summarizes the refactoring...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ration values (stack spacing, direction) - CSS_CLASSES: Consistent CSS class name...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...SS_CLASSES**: Consistent CSS class names - CSS_POSITIONS: Position values (relati...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...**: Position values (relative, absolute) - CSS_CURSORS: Cursor values (grab, grab...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...URSORS**: Cursor values (grab, grabbing) - OPACITY: Opacity values (ghost: 0.3, f...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...**: Opacity values (ghost: 0.3, full: 1) - EVENTS: Custom event names - **FLEX_DI...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...ull: 1) - EVENTS: Custom event names - FLEX_DIRECTIONS: Flexbox direction val...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...X_DIRECTIONS**: Flexbox direction values - DISPLAY_MODES: Display property values...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...DISPLAY_MODES**: Display property values - POINTER_EVENTS: Pointer-events values ...
(QB_NEW_EN)
[grammar] ~66-~66: There might be a mistake here.
Context: ....STACK_SPACING_DEMO ### 4. Test File:test/allonsh.test.js` - Imported constants for event names - Rep...
(QB_NEW_EN)
[grammar] ~70-~70: There might be a mistake here.
Context: ...me with EVENTS.DROP ### 5. CSS File: demo/css/styles.css - Added comments indicating z-index values...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CONSTANTS_REFACTOR_SUMMARY.md
23-23: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
28-28: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
32-32: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
38-38: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
48-48: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
52-52: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
56-56: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (4)
test/allonsh.test.js (1)
3-3: LGTM: tests now reference EVENTS.DROP instead of a string literal.This keeps tests resilient to future renames and centralizes event naming.
Also applies to: 158-159
demo/js/script.js (1)
41-44: LGTM: demo now uses DEFAULTS.STACK_SPACING_DEMO.This removes the last magic “10” from the demo script and centralizes the value.
Also applies to: 147-148
src/allonsh.js (1)
507-528: Revisit DRAG_ENTER/DRAG_LEAVE dispatch timing.Emitting DRAG_ENTER and DRAG_LEAVE immediately after DROP can confuse consumers that expect enter/leave during hover transitions. Consider emitting only DROP here and handling enter/leave in pointer-move logic.
If consumers rely on this behavior, keep as-is; otherwise, I can propose a focused change to raise DRAG_ENTER on first hover and DRAG_LEAVE when the hovered dropzone changes.
CONSTANTS_REFACTOR_SUMMARY.md (1)
1-5: Solid, focused summary.Nicely captures the intent and scope of the refactor; aligns with the PR objectives and keeps API-surface stability explicit.
| _onPointerUp(clientX, clientY, event) { | ||
| this.currentDraggedElement.style.opacity = '1'; | ||
| this.currentDraggedElement.style.opacity = OPACITY.FULL; | ||
| this._handleDrop(clientX, clientY, event); | ||
| } |
There was a problem hiding this comment.
Guard null currentDraggedElement in _onPointerUp.
If _onPointerUp fires without a prior drag, this line will throw before _handleDrop’s guard runs.
Apply this diff:
- _onPointerUp(clientX, clientY, event) {
- this.currentDraggedElement.style.opacity = OPACITY.FULL;
- this._handleDrop(clientX, clientY, event);
- }
+ _onPointerUp(clientX, clientY, event) {
+ if (this.currentDraggedElement) {
+ this.currentDraggedElement.style.opacity = OPACITY.FULL;
+ }
+ this._handleDrop(clientX, clientY, event);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _onPointerUp(clientX, clientY, event) { | |
| this.currentDraggedElement.style.opacity = '1'; | |
| this.currentDraggedElement.style.opacity = OPACITY.FULL; | |
| this._handleDrop(clientX, clientY, event); | |
| } | |
| _onPointerUp(clientX, clientY, event) { | |
| if (this.currentDraggedElement) { | |
| this.currentDraggedElement.style.opacity = OPACITY.FULL; | |
| } | |
| this._handleDrop(clientX, clientY, event); | |
| } |
🤖 Prompt for AI Agents
In src/allonsh.js around lines 304 to 307, _onPointerUp accesses
this.currentDraggedElement.style without ensuring currentDraggedElement is
non-null which can throw; add a null guard: if this.currentDraggedElement is
falsy, skip accessing style (either return early or call this._handleDrop(...)
directly since _handleDrop already guards) otherwise set opacity and then call
this._handleDrop; ensure no direct property access occurs when
currentDraggedElement is null.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/allonsh.js (3)
176-183: Listener binding mismatch leads to leaked handlers and duplicate callbacks._in _initialize you add listeners with fresh
bind(this), but _unbindEventListeners removes different function refs (this._boundMouseDown/_boundTouchStart). The adds won’t be removed, causing leaks and multiple handler invocations after update().- element.addEventListener('mousedown', this._onMouseDown.bind(this)); - element.addEventListener('touchstart', this._onTouchStart.bind(this), { + element.addEventListener('mousedown', this._boundMouseDown); + element.addEventListener('touchstart', this._boundTouchStart, { passive: false, });Also applies to: 205-212
97-115: update(): Guard optional selectors to avoid querying '.undefined' and persist previous selectors.If draggableSelector is omitted, this will query
'.undefined'. Also, the instance does not persist the previous selectors, preventing stable updates.Minimal safe fix:
constructor(options = {}) { const { draggableSelector, dropzoneSelector = null, playAreaSelector = null, @@ if (!draggableSelector) { throw new Error( "Allonsh Error: 'draggableSelector' is required in options." ); } + this.draggableSelector = draggableSelector; + this.dropzoneSelector = dropzoneSelector || null; + this.playAreaSelector = playAreaSelector || null; @@ update(newOptions = {}) { const { draggableSelector, dropzoneSelector, playAreaSelector, @@ - if (playAreaSelector) { + if (playAreaSelector) { const newPlayArea = document.querySelector(`.${playAreaSelector}`); if (newPlayArea) { this.playAreaElement = newPlayArea; - this.playAreaElement.style.position = 'relative'; + this.playAreaElement.style.position = CSS_POSITIONS.RELATIVE; + this.playAreaSelector = playAreaSelector; } else { console.warn( `Allonsh Warning: Play area element with class '${playAreaSelector}' not found.` ); } } this._unbindEventListeners(); - this.draggableElements = this.playAreaElement.querySelectorAll( - `.${draggableSelector}` - ); + if (draggableSelector) this.draggableSelector = draggableSelector; + if (!this.draggableSelector) { + console.warn('Allonsh Warning: No draggableSelector set; skipping rebind.'); + return; + } + this.draggableElements = this.playAreaElement.querySelectorAll( + `.${this.draggableSelector}` + ); @@ - if (dropzoneSelector) { - this.dropzoneElements = this.playAreaElement.querySelectorAll( - `.${dropzoneSelector}` - ); + if (dropzoneSelector !== undefined) { + this.dropzoneSelector = dropzoneSelector || null; + } + if (this.dropzoneSelector) { + this.dropzoneElements = this.playAreaElement.querySelectorAll( + `.${this.dropzoneSelector}` + ); this._dropzoneSet = new Set(this.dropzoneElements); }Also applies to: 116-132, 129-139
707-716: Ensure new dropzones get baseline class and stacking styles.setDropzones updates the internal set but doesn’t add CSS_CLASSES.DROPZONE or apply stacking styles, unlike _initialize(). Align behavior to keep visuals consistent.
setDropzones(selector) { try { this.dropzoneElements = this.playAreaElement.querySelectorAll( `.${selector}` ); this._dropzoneSet = new Set(this.dropzoneElements); + this.dropzoneElements.forEach((dz) => { + dz.classList.add(CSS_CLASSES.DROPZONE); + if (this.enableStacking) this._applyStackingStyles(dz); + else this._removeStackingStyles(dz); + }); } catch (err) { console.error('Allonsh Error setting dropzones:', err); } }
♻️ Duplicate comments (1)
src/allonsh.js (1)
306-309: Guard against null currentDraggedElement in _onPointerUp.This can throw when mouseup/touchend fires without an active drag. This was flagged previously and remains. Add a null check.
- _onPointerUp(clientX, clientY, event) { - this.currentDraggedElement.style.opacity = OPACITY.FULL; - this._handleDrop(clientX, clientY, event); - } + _onPointerUp(clientX, clientY, event) { + if (this.currentDraggedElement) { + this.currentDraggedElement.style.opacity = OPACITY.FULL; + } + this._handleDrop(clientX, clientY, event); + }
🧹 Nitpick comments (12)
src/constants.js (5)
16-22: Reduce duplication in stack direction constants.DEFAULTS contains both the current value (STACK_DIRECTION) and separate vertical/horizontal constants. Prefer a single enum of directions and reference it from DEFAULTS to prevent drift.
Apply in constants.js:
-export const DEFAULTS = { - STACK_SPACING: 5, - STACK_SPACING_DEMO: 10, - STACK_DIRECTION: 'horizontal', - STACK_DIRECTION_VERTICAL: 'vertical', - STACK_DIRECTION_HORIZONTAL: 'horizontal', -}; +export const DIRECTIONS = { + HORIZONTAL: 'horizontal', + VERTICAL: 'vertical', +}; + +export const DEFAULTS = { + STACK_SPACING: 5, + STACK_SPACING_DEMO: 10, + STACK_DIRECTION: DIRECTIONS.HORIZONTAL, +};And in src/allonsh.js update comparisons/usages:
- this.stackDirection === DEFAULTS.STACK_DIRECTION_VERTICAL + this.stackDirection === DIRECTIONS.VERTICALImport DIRECTIONS alongside DEFAULTS where needed.
24-29: Namespace the 'restricted' class to avoid collisions.'restricted' is very generic and may clash with host app CSS. Consider 'allonsh-restricted' (exposed via CSS_CLASSES.RESTRICTED) and provide a deprecation window.
Would you like me to prepare a follow-up PR that introduces 'allonsh-restricted' while still supporting 'restricted' for one minor release?
46-51: Consider namespacing custom events with a colon.To further differentiate from native drag events and third-party libraries, 'allonsh:dragstart' / 'allonsh:drop' patterns are common and compose well.
If you want this, I can generate a codemod to migrate usages and update tests.
58-58: Broaden FLEX_WRAP for future extensibility.Right now it's a single string. If you foresee nowrap or wrap-reverse, model it like DISPLAY_MODES for consistency.
-export const FLEX_WRAP = 'wrap'; +export const FLEX_WRAP = { + WRAP: 'wrap', + NOWRAP: 'nowrap', + WRAP_REVERSE: 'wrap-reverse', +};And use FLEX_WRAP.WRAP in src/allonsh.js where applicable.
6-14: Freeze exported objects to prevent accidental mutation at runtime.Defensive, low-cost hardening for globally shared constants.
Add below the exports:
Object.freeze(Z_INDEX); Object.freeze(DEFAULTS); Object.freeze(CSS_CLASSES); Object.freeze(CSS_POSITIONS); Object.freeze(CSS_CURSORS); Object.freeze(OPACITY); Object.freeze(EVENTS); Object.freeze(FLEX_DIRECTIONS); Object.freeze(DISPLAY_MODES); Object.freeze(POINTER_EVENTS); // If FLEX_WRAP becomes an object per earlier suggestion, freeze it too.CONSTANTS_REFACTOR_SUMMARY.md (3)
105-107: Summary says “no API changes” but src/allonsh.js adds setDropzones().The document claims no API changes, yet a new public method (setDropzones) is introduced. Please reconcile the statement.
- - **Low Risk**: Changes are purely internal refactoring with no API changes + - **Low Risk**: Changes are primarily internal refactoring. One additive API was introduced (`setDropzones(selector)`).
26-26: Fix markdownlint: remove trailing punctuation in headings.Headings ending with a colon violate MD026 in our linter.
-#### Z-Index Values: +#### Z-Index Values -#### Default Values: +#### Default Values -#### CSS Class Names: +#### CSS Class Names -#### CSS Properties: +#### CSS Properties -#### Opacity Values: +#### Opacity Values -#### Pointer Events: +#### Pointer Events -#### Event Names: +#### Event NamesAlso applies to: 32-32, 37-37, 44-44, 55-55, 60-60, 65-65
11-21: Minor grammar polish for list items.Make list items parallel and consistent (e.g., “values” vs “names”) and ensure punctuation/readability.
Proposed edits:
- “Consistent CSS class names” → “CSS class names”
- “Opacity values (ghost: 0.3, full: 1)” → “Opacity (ghost: 0.3, full: 1)”
- “Flexbox direction values” → “Flexbox directions”
- “Display property values” → “Display modes”
- “Pointer-events values” → “Pointer-events”
src/allonsh.js (4)
169-171: Consistency: use constants for 'relative' assignments.Two literals remain; switch to CSS_POSITIONS.RELATIVE for consistency with the refactor.
- this.playAreaElement.style.position = 'relative'; + this.playAreaElement.style.position = CSS_POSITIONS.RELATIVE;And in update():
- this.playAreaElement.style.position = 'relative'; + this.playAreaElement.style.position = CSS_POSITIONS.RELATIVE;Also applies to: 120-121
314-315: Remove redundant cursor assignment in _startDrag.GRABBING is applied twice back-to-back.
- this.currentDraggedElement = event.currentTarget; - this.currentDraggedElement.style.cursor = CSS_CURSORS.GRABBING; + this.currentDraggedElement = event.currentTarget; @@ - this.currentDraggedElement.style.cursor = CSS_CURSORS.GRABBING; + this.currentDraggedElement.style.cursor = CSS_CURSORS.GRABBING;(Keep only one.)
Also applies to: 352-352
509-527: Event semantics: Drop emits enter/leave immediately after drop — intentional?Firing DRAG_ENTER and then DRAG_LEAVE on the same dropzone during a drop may surprise consumers. Typically, enter/leave reflect hover transitions, not drop. Consider emitting only DROP here and handling enter/leave during move, or document this behavior explicitly.
If you want to change this, I can adjust and update tests accordingly.
367-395: _handleElementDrag appears unused.If this isn’t called anywhere, consider removing it to reduce surface area or wire it in where appropriate.
Do you want me to scan the repo and generate a quick usage report?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CONSTANTS_REFACTOR_SUMMARY.md(1 hunks)src/allonsh.js(20 hunks)src/constants.js(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CONSTANTS_REFACTOR_SUMMARY.md
[grammar] ~12-~12: There might be a mistake here.
Context: ...ration values (stack spacing, direction) - CSS_CLASSES: Consistent CSS class name...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...SS_CLASSES**: Consistent CSS class names - CSS_POSITIONS: Position values (relati...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...**: Position values (relative, absolute) - CSS_CURSORS: Cursor values (grab, grab...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...URSORS**: Cursor values (grab, grabbing) - OPACITY: Opacity values (ghost: 0.3, f...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...**: Opacity values (ghost: 0.3, full: 1) - EVENTS: Custom event names - **FLEX_DI...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...ull: 1) - EVENTS: Custom event names - FLEX_DIRECTIONS: Flexbox direction val...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...X_DIRECTIONS**: Flexbox direction values - DISPLAY_MODES: Display property values...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...DISPLAY_MODES**: Display property values - POINTER_EVENTS: Pointer-events values ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CONSTANTS_REFACTOR_SUMMARY.md
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
32-32: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
37-37: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
44-44: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
55-55: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
60-60: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
65-65: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (4)
src/constants.js (1)
6-14: LGTM: Good centralization and cohesive groupings.The constants are well organized, descriptive, and remove magic values from the codebase. The groupings (Z_INDEX, DEFAULTS, CSS_CLASSES, etc.) read cleanly and map directly to usage sites in src/allonsh.js.
Also applies to: 24-29, 31-39, 41-44, 46-51, 53-56, 58-58, 60-63, 65-68
src/allonsh.js (3)
214-223: LGTM: Stacking styles correctly leverage constants.Correct use of DISPLAY_MODES, FLEX_DIRECTIONS, DEFAULTS, and FLEX_WRAP for predictable layout behavior.
334-341: LGTM: Refactor replaces literals with constants across drag/ghost/drop flows.The replacements improve readability and maintainability without changing behavior. Nice coverage across cursor, z-index, position, pointer-events, and classes.
Also applies to: 352-354, 375-383, 421-422, 553-558, 599-599, 606-606, 643-643, 651-652, 674-684, 695-701
463-466: Smart use of pointer-events toggling to probe elementBelow.This pattern avoids hit-testing the dragged element itself. Solid approach.
| import { | ||
| Z_INDEX, | ||
| DEFAULTS, | ||
| CSS_CLASSES, | ||
| CSS_POSITIONS, | ||
| CSS_CURSORS, | ||
| OPACITY, | ||
| EVENTS, | ||
| FLEX_DIRECTIONS, | ||
| FLEX_WRAP, | ||
| DISPLAY_MODES, | ||
| POINTER_EVENTS, | ||
| } from './constants.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sanity check: great coverage of constants usage; verify no remaining literals slipped through.
Quick repo scan can ensure no more magic strings for the refactored categories.
Also applies to: 24-25, 133-139, 176-179, 194-195, 216-223, 375-383, 421-422, 463-466, 553-558, 599-599, 606-606, 643-643, 651-652, 674-684, 695-701
🏁 Script executed:
#!/bin/bash
# From repo root. Lists any remaining literals for known categories outside constants.js.
# Adjust patterns as needed.
set -euo pipefail
# Exclude node_modules, dist, build, coverage
EXCLUDES='-g !**/node_modules/** -g !**/dist/** -g !**/build/** -g !**/coverage/**'
# 1) CSS class names
rg -nP $EXCLUDES "(allonsh-(dropzone|highlight|draggable))" --glob '!src/constants.js'
# 2) Event names
rg -nP $EXCLUDES "(allonsh-(dragstart|drop|dragenter|dragleave))" --glob '!src/constants.js'
# 3) CSS values that should be constants
rg -nP $EXCLUDES "\\b(relative|absolute|grab|grabbing|flex|row|column|wrap|none)\\b" --glob '!src/constants.js'
# 4) Opacity/z-index common literals
rg -nP $EXCLUDES "\\b(0\\.3|1|9999|1000|999)\\b" --glob '!src/constants.js'Length of output: 25925
Replace remaining literal strings with centralized constants
A quick ripgrep scan confirms that several magic strings still exist outside of src/constants.js. Please refactor the following:
- src/allonsh.js
• Lines 120 & 169:→ replacethis.playAreaElement.style.position = 'relative';
'relative'withCSS_POSITIONS.RELATIVE. - test/allonsh.test.js
• Cursor, display and position assertions hardcode values:→ import and useexpect(el.style.cursor).toBe('grab'); expect(el.style.display).toBe('flex'); expect(allonsh.ghostElement.style.position).toBe('absolute');
CSS_CURSORS.GRAB,DISPLAY_MODES.FLEXandCSS_POSITIONS.ABSOLUTEin your expected values. - Demo & docs (
demo/js/script.js,README.md,docs/allonsh.md, etc.)
• Literal class names ('allonsh-draggable','allonsh-dropzone','allonsh-highlight') and event names ('allonsh-dragstart','allonsh-drop', etc.) remain—update these examples to referenceCSS_CLASSESandEVENTSconstants. - CONSTANTS_REFACTOR_SUMMARY.md
• Remove duplicate mapping lines for CSS classes and events now centralized inconstants.js. - (Optional) Consider migrating hard-coded z-index and opacity in
demo/css/styles.cssto CSS custom properties (see note in summary).
This will ensure no “magic” literals slip through and keeps all values truly centralized.
🤖 Prompt for AI Agents
In src/allonsh.js around lines 120 and 169, replace the hard-coded string
assignments this.playAreaElement.style.position = 'relative' with the
centralized constant this.playAreaElement.style.position =
CSS_POSITIONS.RELATIVE (CSS_POSITIONS is already imported at the top); update
both occurrences exactly and run tests to verify no other literal 'relative'
usages remain.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/allonsh.js (2)
176-183: Use the same bound handlers everywhere to avoid listener leaks.
_initializeusesbind(...)inline, whileupdate()uses pre-bound handlers. You won’t be able to remove the inline-bound listeners in_unbindEventListeners, causing duplicated handlers after updates.- this.draggableElements.forEach((element) => { + this.draggableElements.forEach((element) => { try { - element.style.cursor = CSS_CURSORS.GRAB; - element.addEventListener('mousedown', this._onMouseDown.bind(this)); - element.addEventListener('touchstart', this._onTouchStart.bind(this), { - passive: false, - }); + element.style.cursor = CSS_CURSORS.GRAB; + element.addEventListener('mousedown', this._boundMouseDown); + element.addEventListener('touchstart', this._boundTouchStart, { + passive: false, + }); } catch (err) {
97-116:update()breaks whendraggableSelectorisn’t provided.You re-query draggables with an undefined selector (lines 129–131), producing
'.undefined'. Persist the original selectors and fall back to them.class Allonsh { constructor(options = {}) { const { draggableSelector, dropzoneSelector = null, @@ } = options; @@ + // Persist selectors for future updates + this.draggableSelector = draggableSelector; + this.dropzoneSelector = dropzoneSelector; @@ update(newOptions = {}) { const { draggableSelector, dropzoneSelector, @@ } = newOptions; @@ - this.draggableElements = this.playAreaElement.querySelectorAll( - `.${draggableSelector}` - ); + const ds = + draggableSelector ?? this.draggableSelector ?? CSS_CLASSES.DRAGGABLE; + this.draggableSelector = ds; + this.draggableElements = this.playAreaElement.querySelectorAll(`.${ds}`); @@ - if (dropzoneSelector) { - this.dropzoneElements = this.playAreaElement.querySelectorAll( - `.${dropzoneSelector}` - ); - this._dropzoneSet = new Set(this.dropzoneElements); - } + const zs = dropzoneSelector ?? this.dropzoneSelector; + if (zs) { + this.dropzoneSelector = zs; + this.dropzoneElements = this.playAreaElement.querySelectorAll(`.${zs}`); + this._dropzoneSet = new Set(this.dropzoneElements); + }
♻️ Duplicate comments (1)
src/allonsh.js (1)
306-309: Guard against nullcurrentDraggedElementin_onPointerUp.This throws if pointer-up occurs without an active drag (race/edge cases). This was flagged previously and remains unfixed.
Apply this diff:
_onPointerUp(clientX, clientY, event) { - this.currentDraggedElement.style.opacity = OPACITY.FULL; + if (this.currentDraggedElement) { + this.currentDraggedElement.style.opacity = OPACITY.FULL; + } this._handleDrop(clientX, clientY, event); }
🧹 Nitpick comments (11)
demo/js/script.js (2)
6-6: Use centralized class-name constants for selectors.These are still hardcoded at Lines 33–35. For consistency with the refactor and to avoid drift between demo markup and library, source them from
CSS_CLASSES.Apply this diff:
-const draggableSelectorClass = 'draggable'; -const dropzoneSelectorClass = 'dropzone'; +const draggableSelectorClass = CSS_CLASSES.DRAGGABLE; +const dropzoneSelectorClass = CSS_CLASSES.DROPZONE;
148-149: Use DEFAULTS for the reset state.Hardcoding
'horizontal'undermines the constants centralization. UseDEFAULTS.STACK_DIRECTIONinstead.- currentStackDirection = 'horizontal'; + currentStackDirection = DEFAULTS.STACK_DIRECTION;demo/css/styles.css (2)
296-299: Use a freemode-specific z-index token for the freemode title.This block uses
--allonsh-z-index-control-panelbut elsewhere (.toggle-panel-btn) a dedicated--allonsh-z-index-freemode-titleis referenced. Aligning both to the freemode token avoids cross-coupling unrelated layers.- z-index: var( - --allonsh-z-index-control-panel, - 5 - ); /* migrated to CSS custom property */ + z-index: var( + --allonsh-z-index-freemode-title, + 5 + ); /* migrated to CSS custom property */
460-463: Prefer a control-panel-specific z-index token in the mobile layout.On mobile,
.demo-control-paneluses--allonsh-z-index-theme-toggle. Consider a dedicated control-panel token to avoid coupling with the theme toggle’s layer.- z-index: var( - --allonsh-z-index-theme-toggle, - 9 - ); /* migrated to CSS custom property */ + z-index: var( + --allonsh-z-index-control-panel, + 9 + ); /* migrated to CSS custom property */CONSTANTS_REFACTOR_SUMMARY.md (4)
11-21: Include all exported groups and tighten prose.
FLEX_WRAPis referenced later but not listed here. Also minor grammar cleanups improve readability.- **DISPLAY_MODES**: Display property values - **POINTER_EVENTS**: Pointer-events values +- **FLEX_WRAP**: Flex wrapping values
26-31: Remove trailing punctuation in headings.Headings ending with
:trip MD026. Drop the trailing colons.-#### Z-Index Values: +#### Z-Index Values @@ -#### Default Values: +#### Default Values @@ -#### CSS Class Names: +#### CSS Class Names @@ -#### CSS Properties: +#### CSS Properties @@ -#### Opacity Values: +#### Opacity Values @@ -#### Pointer Events: +#### Pointer Events @@ -#### Event Names: +#### Event NamesAlso applies to: 32-36, 37-41, 41-41, 52-56, 57-61, 62-64
62-64: Fill in the missing event mappings.The “Event Names” section header has no content; add the mappings for completeness.
#### Event Names +- `'allonsh-dragstart'` → `EVENTS.DRAG_START` +- `'allonsh-drop'` → `EVENTS.DROP` +- `'allonsh-dragenter'` → `EVENTS.DRAG_ENTER` +- `'allonsh-dragleave'` → `EVENTS.DRAG_LEAVE`
74-78: Update CSS notes to reflect the current migration.The CSS file now uses CSS custom properties with fallbacks, not “values remain hardcoded.”
- - Added comments indicating z-index values could be updated to use CSS custom properties - - Values remain hardcoded for now but are documented for future refactoring + - Introduced CSS custom properties (with fallbacks) for z-index and opacity in demo CSS to enable theming while preserving current visualsdocs/allonsh.md (1)
83-96: Optional: demonstrate constants for stack config.To reinforce adoption, consider showing
FLEX_DIRECTIONS.COLUMNandDEFAULTS.STACK_SPACING.import Allonsh from 'allonsh'; -import { CSS_CLASSES, EVENTS } from 'allonsh/constants'; +import { CSS_CLASSES, EVENTS, FLEX_DIRECTIONS, DEFAULTS } from 'allonsh/constants'; @@ enableStacking: true, - stackDirection: 'vertical', - stackSpacing: 10, + stackDirection: FLEX_DIRECTIONS.COLUMN, + stackSpacing: DEFAULTS.STACK_SPACING, useGhostEffect: true,src/allonsh.js (2)
509-527: Revisit event semantics on drop.Dispatching
DRAG_ENTERandDRAG_LEAVEimmediately afterDROPis atypical and may mislead consumers expecting those to reflect hover transitions. Unless tests or downstream depend on this, consider emitting onlyDROPhere and using enter/leave during hover.Would you like a patch that moves enter/leave dispatching into the pointer-move path with state tracking?
603-625: Restore opacity on reset for robustness.Opacity is set to
OPACITY.GHOSTduring drag, and normally restored in_onPointerUp, but adding a reset here prevents stale styles in edge cases._resetDraggedElementState() { if (!this.currentDraggedElement) return; @@ - this.currentDraggedElement.style.cursor = CSS_CURSORS.GRAB; + this.currentDraggedElement.style.cursor = CSS_CURSORS.GRAB; + this.currentDraggedElement.style.opacity = OPACITY.FULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
CONSTANTS_REFACTOR_SUMMARY.md(1 hunks)README.md(1 hunks)demo/css/styles.css(8 hunks)demo/js/script.js(6 hunks)docs/allonsh.md(1 hunks)src/allonsh.js(22 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🪛 Biome (2.1.2)
demo/js/script.js
[error] 42-42: Shouldn't redeclare 'DEFAULTS'. Consider to delete it or rename it.
'DEFAULTS' is defined here:
(lint/suspicious/noRedeclare)
🪛 LanguageTool
CONSTANTS_REFACTOR_SUMMARY.md
[grammar] ~12-~12: There might be a mistake here.
Context: ...ration values (stack spacing, direction) - CSS_CLASSES: Consistent CSS class name...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...SS_CLASSES**: Consistent CSS class names - CSS_POSITIONS: Position values (relati...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...**: Position values (relative, absolute) - CSS_CURSORS: Cursor values (grab, grab...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...URSORS**: Cursor values (grab, grabbing) - OPACITY: Opacity values (ghost: 0.3, f...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...**: Opacity values (ghost: 0.3, full: 1) - EVENTS: Custom event names - **FLEX_DI...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...ull: 1) - EVENTS: Custom event names - FLEX_DIRECTIONS: Flexbox direction val...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...X_DIRECTIONS**: Flexbox direction values - DISPLAY_MODES: Display property values...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...DISPLAY_MODES**: Display property values - POINTER_EVENTS: Pointer-events values ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CONSTANTS_REFACTOR_SUMMARY.md
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
32-32: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
37-37: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
41-41: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
52-52: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
57-57: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
62-62: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (10)
demo/js/script.js (2)
183-196: LGTM on constants-driven selectors.Using
CSS_CLASSES.DRAGGABLE/CSS_CLASSES.DROPZONEhere eliminates magic strings and aligns the demo script with the central constants.
212-212: LGTM on centralizing the emptying selector.Swapping
'.dropzone'forCSS_CLASSES.DROPZONEis consistent with the refactor goals.demo/css/styles.css (5)
60-63: Good migration to a CSS custom property with fallback.The header’s z-index now respects
--allonsh-z-index-headerwhile keeping a sane default. Nice forward-compat for theming.
218-219: Opacity tokenization looks good.
--allonsh-opacity-demowith a fallback preserves existing visuals and enables theme overrides.
232-235: Consistent z-index token for floating controls.Using
--allonsh-z-index-draggingfor the toggle button keeps layering aligned with drag state.
366-367: LGTM on using--allonsh-z-index-freemode-titlefor the toggle.Consistent with the suggested change above.
439-443: Good: themable z-index for the theme toggle.Token + fallback preserves current stack while enabling future overrides.
docs/allonsh.md (1)
98-105: Nice: examples use EVENTS and CSS_CLASSES.Using constants in examples reduces copy/paste errors and keeps docs in sync with the code.
src/allonsh.js (2)
169-170: LGTM on centralizing position values.Replacing string literals with
CSS_POSITIONS.RELATIVEkeeps styling consistent.Also applies to: 642-643
334-341: Broad constants adoption looks solid.
- Pointer events, positions, z-indexes, and opacities use centralized enums.
- Class additions/removals and queries consistently use
CSS_CLASSES.- Display, flex direction/wrap, and cursors use their respective constants.
Also applies to: 367-383, 421-421, 553-558, 598-600, 650-652, 673-685, 695-701
| const openPanelBtn = document.getElementById('openPanelBtn'); | ||
| const closePanelBtn = document.getElementById('closePanelBtn'); | ||
|
|
||
| import { DEFAULTS, CSS_CLASSES, EVENTS } from '../../src/constants.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix duplicate import and remove unused identifier.
DEFAULTS is imported twice (Lines 6 and 42), triggering a linter error. Also, EVENTS is imported but never used.
Apply this diff:
-import { DEFAULTS, CSS_CLASSES, EVENTS } from '../../src/constants.js';
+import { DEFAULTS, CSS_CLASSES } from '../../src/constants.js';
@@
-import { DEFAULTS } from '../../src/constants.js';Also applies to: 42-42
🤖 Prompt for AI Agents
In demo/js/script.js around lines 6 and 42, there is a duplicate import of
DEFAULTS (imported at line 6 and again at line 42) and EVENTS is imported but
never used; remove the duplicate import and drop the unused EVENTS identifier so
the module only imports each symbol once and no unused symbols remain (keep a
single import that includes DEFAULTS and CSS_CLASSES as needed and remove the
second import at line 42 and the unused EVENTS from the import list).
There was a problem hiding this comment.
Hi @p55d2k, please address this issue as it is breaking the demo.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| const draggableSelectorClass = CSS_CLASSES.DRAGGABLE; | ||
| const dropzoneSelectorClass = CSS_CLASSES.DROPZONE; | ||
| } |
There was a problem hiding this comment.
Restore button state toggling for horizontal case.
The else-branch no longer toggles .selected classes; instead it declares unused locals. This regresses the UI feedback when switching back to horizontal.
Apply this diff:
} else {
- const draggableSelectorClass = CSS_CLASSES.DRAGGABLE;
- const dropzoneSelectorClass = CSS_CLASSES.DROPZONE;
+ horizontalStackBtn.classList.add('selected');
+ verticalStackBtn.classList.remove('selected');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const draggableSelectorClass = CSS_CLASSES.DRAGGABLE; | |
| const dropzoneSelectorClass = CSS_CLASSES.DROPZONE; | |
| } | |
| } else { | |
| horizontalStackBtn.classList.add('selected'); | |
| verticalStackBtn.classList.remove('selected'); | |
| } |
🤖 Prompt for AI Agents
In demo/js/script.js around lines 101 to 103, the else-branch currently declares
unused locals (const draggableSelectorClass = CSS_CLASSES.DRAGGABLE; const
dropzoneSelectorClass = CSS_CLASSES.DROPZONE;) and no longer toggles the
'.selected' class for the horizontal layout, which breaks UI feedback; replace
those unused declarations with logic that finds the relevant draggable and
dropzone elements for the horizontal case and toggles (add/remove) the
CSS_CLASSES.SELECTED (or '.selected') class on them to match the behavior in the
other branch so that switching back to horizontal restores selected state.
There was a problem hiding this comment.
@p55d2k, Please revert these changes so that the class toggle continues to function correctly.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| import { CSS_CLASSES, EVENTS } from 'allonsh/constants'; | ||
|
|
||
| const dragDrop = new Allonsh({ | ||
| draggableSelector: 'draggable', | ||
| dropzoneSelector: 'dropzone', | ||
| draggableSelector: CSS_CLASSES.DRAGGABLE, | ||
| dropzoneSelector: CSS_CLASSES.DROPZONE, | ||
| playAreaSelector: 'play-area', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that allonsh/constants is actually exported.
The docs import from a subpath. Ensure your package.json “exports” includes "./constants"; otherwise consumers cannot import it.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
jq '.exports' package.json || { echo "No exports field"; exit 1; }
echo "Looking for ./constants subpath…"
jq -r '.exports|tostring' package.json | rg -n "\./constants" -n || echo "Missing ./constants export"Length of output: 256
🏁 Script executed:
#!/bin/bash
# Find all files named constants.* in the repository
fd -t f 'constants.*'Length of output: 70
Add subpath export for constants in package.json
The import import { CSS_CLASSES, EVENTS } from 'allonsh/constants' will fail because your package.json currently has no “exports” field (jq returned null), so consumers can’t resolve the constants subpath even though src/constants.js exists.
• File to update: package.json
• Action: add an exports map that exposes both the package root and the ./constants subpath.
Suggested diff (adjust paths to match your bundle outputs):
{
"name": "allonsh",
"version": "1.0.0",
- "main": "src/index.js",
+ "main": "src/index.js",
+ "exports": {
+ ".": "./src/index.js",
+ "./constants": "./src/constants.js"
+ },
// …other fields…
}After this change, import { CSS_CLASSES } from 'allonsh/constants' will correctly resolve to your src/constants.js.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { CSS_CLASSES, EVENTS } from 'allonsh/constants'; | |
| const dragDrop = new Allonsh({ | |
| draggableSelector: 'draggable', | |
| dropzoneSelector: 'dropzone', | |
| draggableSelector: CSS_CLASSES.DRAGGABLE, | |
| dropzoneSelector: CSS_CLASSES.DROPZONE, | |
| playAreaSelector: 'play-area', | |
| { | |
| "name": "allonsh", | |
| "version": "1.0.0", | |
| "main": "src/index.js", | |
| "exports": { | |
| ".": "./src/index.js", | |
| "./constants": "./src/constants.js" | |
| }, | |
| // …other fields… | |
| } |
🤖 Prompt for AI Agents
In docs/allonsh.md around lines 85 to 90, the example import from
'allonsh/constants' will fail because package.json lacks an "exports" map
exposing the ./constants subpath; update package.json to add an "exports" field
that maps the package root (".") and the "./constants" subpath to the
appropriate built files (e.g., "./dist/index.js" and "./dist/constants.js" or
your bundle output paths), ensuring both CommonJS and ESM entry points if needed
(provide "./dist/... .js" and "./dist/... .mjs" or use "require" and "import"
fields), and bump the package.json accordingly so import { CSS_CLASSES, EVENTS }
from 'allonsh/constants' resolves correctly.
2 participants
Allonsh Constants Refactor Summary
Overview
This document summarizes the refactoring work done to replace hardcoded values with meaningful constants throughout the Allonsh library.
Files Created/Modified
1. New Constants File:
src/constants.js2. Main Library:
src/allonsh.jsReplaced hardcoded values with constants:
Z-Index Values:
999→Z_INDEX.GHOST1000→Z_INDEX.DRAGGING9999→Z_INDEX.DROPPEDDefault Values:
'horizontal'→DEFAULTS.STACK_DIRECTION5→DEFAULTS.STACK_SPACINGCSS Class Names:
'allonsh-dropzone'→CSS_CLASSES.DROPZONE'allonsh-highlight'→CSS_CLASSES.HIGHLIGHT'allonsh-draggable'→CSS_CLASSES.DRAGGABLE'restricted'→CSS_CLASSES.RESTRICTEDCSS Properties:
'relative'→CSS_POSITIONS.RELATIVE'absolute'→CSS_POSITIONS.ABSOLUTE'grab'→CSS_CURSORS.GRAB'grabbing'→CSS_CURSORS.GRABBING'flex'→DISPLAY_MODES.FLEX'row'→FLEX_DIRECTIONS.ROW'column'→FLEX_DIRECTIONS.COLUMN'wrap'→FLEX_WRAPOpacity Values:
'0.3'→OPACITY.GHOST'1'→OPACITY.FULLPointer Events:
'none'→POINTER_EVENTS.NONE'auto'→POINTER_EVENTS.AUTOEvent Names:
'allonsh-dragstart'→EVENTS.DRAG_START'allonsh-drop'→EVENTS.DROP'allonsh-dragenter'→EVENTS.DRAG_ENTER'allonsh-dragleave'→EVENTS.DRAG_LEAVE3. Demo Script:
demo/js/script.js10withDEFAULTS.STACK_SPACING_DEMO4. Test File:
test/allonsh.test.jsEVENTS.DROP5. CSS File:
demo/css/styles.cssBenefits Achieved
Future Improvements
Impact
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores