Skip to content

fix: refactor constants#28

Merged
satnam72 merged 1 commit intosatnam72:devfrom
p55d2k:fix/constants-refactor
Aug 27, 2025
Merged

fix: refactor constants#28
satnam72 merged 1 commit intosatnam72:devfrom
p55d2k:fix/constants-refactor

Conversation

@p55d2k
Copy link

@p55d2k p55d2k commented Aug 27, 2025

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.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
  • DISPLAY_MODES: Display property values
  • POINTER_EVENTS: Pointer-events values

2. Main Library: src/allonsh.js

Replaced hardcoded values with constants:

Z-Index Values:

  • 999Z_INDEX.GHOST
  • 1000Z_INDEX.DRAGGING
  • 9999Z_INDEX.DROPPED

Default Values:

  • 'horizontal'DEFAULTS.STACK_DIRECTION
  • 5DEFAULTS.STACK_SPACING

CSS Class Names:

  • 'allonsh-dropzone'CSS_CLASSES.DROPZONE
  • 'allonsh-highlight'CSS_CLASSES.HIGHLIGHT
  • 'allonsh-draggable'CSS_CLASSES.DRAGGABLE
  • 'restricted'CSS_CLASSES.RESTRICTED

CSS 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_WRAP

Opacity Values:

  • '0.3'OPACITY.GHOST
  • '1'OPACITY.FULL

Pointer Events:

  • 'none'POINTER_EVENTS.NONE
  • 'auto'POINTER_EVENTS.AUTO

Event Names:

  • 'allonsh-dragstart'EVENTS.DRAG_START
  • 'allonsh-drop'EVENTS.DROP
  • 'allonsh-dragenter'EVENTS.DRAG_ENTER
  • 'allonsh-dragleave'EVENTS.DRAG_LEAVE

3. Demo Script: demo/js/script.js

  • Imported constants for default values
  • Replaced hardcoded 10 with DEFAULTS.STACK_SPACING_DEMO

4. Test File: test/allonsh.test.js

  • Imported constants for event names
  • Replaced hardcoded event name with EVENTS.DROP

5. CSS File: demo/css/styles.css

  • 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

Benefits Achieved

  1. Maintainability: All hardcoded values are now centralized in one location
  2. Consistency: Values are guaranteed to be consistent across the codebase
  3. Self-documenting: Constants have meaningful names that explain their purpose
  4. Easy Updates: Changing a value requires updating only the constants file
  5. Type Safety: Better IDE support and error detection
  6. Code Clarity: Intent is clearer with named constants instead of magic numbers

Future Improvements

  1. CSS Custom Properties: Z-index values in CSS could be moved to CSS custom properties
  2. Theme System: Constants could be extended to support theme-based values
  3. Configuration: Constants could be made configurable at runtime
  4. Validation: Add validation to ensure constant values are within expected ranges

Impact

  • High Priority: This was a quick win with massive maintainability improvement
  • Low Risk: Changes are purely internal refactoring with no API changes
  • High Value: Makes the codebase significantly easier to maintain and extend

Summary by CodeRabbit

  • New Features
    • Public constants are now available for CSS classes, events, positions, cursors, z-index, opacity, display, flex, and pointer events. Use them to configure behavior and listen for drag/drop events. No breaking changes.
  • Refactor
    • Replaced hard-coded values with centralized constants for consistency and easier configuration, without altering functionality.
  • Documentation
    • Updated examples to import and use constants; added a dedicated constants reference and event-listening examples.
  • Tests
    • Updated tests to use event constants.

@coderabbitai
Copy link

coderabbitai bot commented Aug 27, 2025

Walkthrough

Centralizes hardcoded strings into a new src/constants.js and refactors src/allonsh.js to use them. Updates README and docs to import and reference these constants, including custom event names. Tests switch to event constants. Adds a new docs/constants.md describing all constants. No public method signatures changed.

Changes

Cohort / File(s) Summary
Core refactor to constants
src/allonsh.js
Replaced inline strings with imports from constants for classes, positions, cursors, z-index, opacity, display/flex, pointer-events, defaults, and custom event names. Internal logic and queries updated to rely on constants; public API unchanged.
New constants module (exports)
src/constants.js
Added new module exporting Z_INDEX, DEFAULTS, CSS_CLASSES, CSS_POSITIONS, CSS_CURSORS, OPACITY, EVENTS, FLEX_DIRECTIONS, FLEX_WRAP, DISPLAY_MODES, POINTER_EVENTS. Establishes single source of truth for UI strings and config.
Documentation updates
README.md, docs/allonsh.md, docs/constants.md
README and docs/allonsh updated to import/use CSS_CLASSES and EVENTS in examples. New docs/constants.md catalogs all constants, their values, and usage examples.
Tests update to constants
test/allonsh.test.js
Replaced hardcoded 'allonsh-drop' with EVENTS.DROP; imported EVENTS from constants.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant A as Allonsh (src/allonsh.js)
  participant D as Draggable (.DRAGGABLE)
  participant Z as Dropzone (.DROPZONE)
  participant DOC as Document

  Note over A,D: Initialization uses CSS_CLASSES, DEFAULTS, DISPLAY_MODES

  U->>D: Mouse/touch press
  A->>D: Set cursor = CSS_CURSORS.GRABBING<br/>Apply OPACITY.GHOST (ghost)
  A-->>DOC: dispatch EVENTS.DRAG_START
  D-->>Z: Move over dropzone
  A->>Z: Toggle highlight (CSS_CLASSES.HIGHLIGHT)
  alt Leaves dropzone
    A->>Z: Remove highlight
    A-->>DOC: dispatch EVENTS.DRAG_LEAVE
  else Drops on dropzone
    U->>D: Release
    A->>Z: Place draggable (flex layout via DISPLAY_MODES/FLEX_DIRECTIONS)
    A-->>DOC: dispatch EVENTS.DROP
    A->>D: Reset cursor = CSS_CURSORS.GRAB<br/>OPACITY.FULL
  end

  Note over A: Positions via CSS_POSITIONS, z-index via Z_INDEX, pointer via POINTER_EVENTS
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I tucked my strings in constant nests,
With z-index dreams and cursor quests;
Events now hop with named delight,
Drag to drop in tidy light.
Docs and tests all join the troupe—
Thump-thump! says rabbit, “What a loop!” 🐇✨

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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: Use bound handlers to avoid leaks and duplicate listeners.
_event listeners are added with fresh bind() references, but removed via cached bound refs in unbindEventListeners() — they won’t match.

Apply:

-        element.addEventListener('mousedown', this._onMouseDown.bind(this));
-        element.addEventListener('touchstart', this._onTouchStart.bind(this), {
-          passive: false,
-        });
+        element.addEventListener('mousedown', this._boundMouseDown);
+        element.addEventListener('touchstart', this._boundTouchStart, {
+          passive: false,
+        });

509-527: Don’t emit DRAG_ENTER/DRAG_LEAVE on drop; semantics are wrong.
Firing ENTER then immediately LEAVE on the same dropzone after DROP will confuse listeners and may break UX.

Remove these dispatches here and emit enter/leave during movement when the hovered dropzone actually changes.

-        this.currentDropzone.dispatchEvent(
-          new CustomEvent(EVENTS.DRAG_ENTER, {
-            detail: { draggedElement: this.currentDraggedElement },
-          })
-        );
-        this.currentDropzone.dispatchEvent(
-          new CustomEvent(EVENTS.DRAG_LEAVE, {
-            detail: { draggedElement: this.currentDraggedElement },
-          })
-        );

If needed, track lastHoveredDropzone in _updateDragPosition and dispatch enter/leave on transitions.


116-147: Preserve persistent selectors and apply DROPZONE class in update()

The current update() implementation always uses the local draggableSelector/dropzoneSelector from newOptions and never stores the original selectors on this. As a result:

  • Calling update() without draggableSelector leads to querying ".undefined", clearing out draggables.
  • Newly queried dropzones in update() aren’t given CSS_CLASSES.DROPZONE, so they miss the expected styling/behavior.

To fix, you should:

  • In the constructor (after validating draggableSelector):
    this.draggableSelector = draggableSelector;
  • In update(newOptions):
    if (draggableSelector !== undefined) {
      this.draggableSelector = draggableSelector;
    }
  • Change the query for draggables to use the instance property:
       // Old:
  • this.draggableElements = this.playAreaElement.querySelectorAll(
  •  `.${draggableSelector}`
    
  • );
    // New:
  • this.draggableElements = this.playAreaElement.querySelectorAll(
  •  `.${this.draggableSelector}`
    
  • );
- After re-querying dropzones, add the DROPZONE class:
```diff
   if (dropzoneSelector) {
     this.dropzoneElements = this.playAreaElement.querySelectorAll(
       `.${dropzoneSelector}`
     );
     this._dropzoneSet = new Set(this.dropzoneElements);
+      this.dropzoneElements.forEach(dz =>
+        dz.classList.add(CSS_CLASSES.DROPZONE)
+      );
   }

These changes ensure selectors persist across updates and newly discovered dropzones behave consistently.

🧹 Nitpick comments (11)
src/constants.js (4)

6-14: Freeze exported objects to prevent accidental mutation at runtime.

Shallow-freezing keeps the constants truly constant without API impact.

Apply:

-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, 60-63, 65-68


16-22: Avoid mixing defaults with enum-like values under DEFAULTS.

Move vertical/horizontal to a dedicated STACK_DIRECTIONS enum and reference it from DEFAULTS.

Apply:

 export const DEFAULTS = Object.freeze({
   STACK_SPACING: 5,
   STACK_SPACING_DEMO: 10,
-  STACK_DIRECTION: 'horizontal',
-  STACK_DIRECTION_VERTICAL: 'vertical',
-  STACK_DIRECTION_HORIZONTAL: 'horizontal',
+  STACK_DIRECTION: 'horizontal',
 });
+
+export const STACK_DIRECTIONS = Object.freeze({
+  HORIZONTAL: 'horizontal',
+  VERTICAL: 'vertical',
+});

Then, where used, prefer STACK_DIRECTIONS.HORIZONTAL and STACK_DIRECTIONS.VERTICAL.


24-29: Consider namespacing RESTRICTED for consistency.

RESTRICTED: 'restricted' may collide with user styles; consider 'allonsh-restricted'. Treat as a future deprecation to avoid breaking changes.


58-58: Consider expanding FLEX_WRAP to an enum.

Expose NOWRAP/WRAP/WRAP_REVERSE for completeness, mirroring other groups.

Example:

export const FLEX_WRAP = Object.freeze({
  NOWRAP: 'nowrap',
  WRAP: 'wrap',
  WRAP_REVERSE: 'wrap-reverse',
});
README.md (2)

36-59: Showcase constants for stackDirection/stackSpacing in the example.

Improves discoverability and consistency with the new constants module.

Apply:

-import Allonsh from './allonsh.js';
-import { CSS_CLASSES, EVENTS } from './constants.js';
+import Allonsh from './allonsh.js';
+import { CSS_CLASSES, EVENTS, DEFAULTS } from './constants.js';
+import { STACK_DIRECTIONS } from './constants.js'; // if added per suggestion

 // Initialize Allonsh with options
 const allonsh = new Allonsh({
   draggableSelector: CSS_CLASSES.DRAGGABLE, // Required: class for draggable elements
   dropzoneSelector: CSS_CLASSES.DROPZONE, // Optional: class for dropzones
   playAreaSelector: 'play-area', // Optional: class for the container area
   restrictToDropzones: false, // Optional: restrict dragging to dropzones
   enableStacking: true, // Optional: enable stacking inside dropzones
-  stackDirection: 'horizontal', // Optional: 'horizontal' or 'vertical'
-  stackSpacing: 8, // Optional: spacing between stacked items (px)
+  stackDirection: DEFAULTS.STACK_DIRECTION, // or STACK_DIRECTIONS.HORIZONTAL
+  stackSpacing: DEFAULTS.STACK_SPACING, // or DEFAULTS.STACK_SPACING_DEMO
   useGhostEffect: true, // Optional: enable ghost effect (shows transparent clone while dragging)
 });

36-39: Align import paths across docs and README.

README uses relative paths while docs use package-style imports. Pick one style or note both to prevent confusion.

docs/constants.md (1)

101-115: Unify import path style with other docs.

Elsewhere you import from allonsh/constants; mirror that here for consistency.

Apply:

-import {
+import {
   Z_INDEX,
   DEFAULTS,
   CSS_CLASSES,
   CSS_POSITIONS,
   CSS_CURSORS,
   OPACITY,
   EVENTS,
   FLEX_DIRECTIONS,
   FLEX_WRAP,
   DISPLAY_MODES,
   POINTER_EVENTS,
-} from './constants.js';
+} from 'allonsh/constants';
docs/allonsh.md (1)

84-96: Optionally use constants in the example for direction/spacing.

Small polish to reinforce the centralized constants pattern.

Apply:

-import { CSS_CLASSES, EVENTS } from 'allonsh/constants';
+import { CSS_CLASSES, EVENTS, DEFAULTS } from 'allonsh/constants';
+import { STACK_DIRECTIONS } from 'allonsh/constants'; // if added

 ...
   restrictToDropzones: true,
   enableStacking: true,
-  stackDirection: 'vertical',
-  stackSpacing: 10,
+  stackDirection: STACK_DIRECTIONS.VERTICAL,
+  stackSpacing: DEFAULTS.STACK_SPACING_DEMO,
test/allonsh.test.js (1)

40-51: Minor: assert more styling via constants in future tests.

Optionally use DISPLAY_MODES/FLEX_DIRECTIONS to check styles semantically rather than string literals.

src/allonsh.js (2)

312-355: Remove duplicate cursor assignment.
GRABBING is set at Line 314 and again at Line 352.

@@
-      this.currentDraggedElement.style.cursor = CSS_CURSORS.GRABBING;
       this.currentDraggedElement.style.zIndex = Z_INDEX.DRAGGING;

397-437: Unify clamping math across ghost/non-ghost paths.
Ghost path uses getBoundingClientRect().width/height; non-ghost path uses clientWidth/clientHeight. Mixed bases can cause off-by-border discrepancies.

-        const maxLeft = this.playAreaElement.clientWidth - draggedRect.width;
-        const maxTop = this.playAreaElement.clientHeight - draggedRect.height;
+        const maxLeft = playAreaRect.width - draggedRect.width;
+        const maxTop = playAreaRect.height - draggedRect.height;
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 045684f and bd6d0a1.

📒 Files selected for processing (6)
  • README.md (1 hunks)
  • docs/allonsh.md (1 hunks)
  • docs/constants.md (1 hunks)
  • src/allonsh.js (22 hunks)
  • src/constants.js (1 hunks)
  • test/allonsh.test.js (2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/constants.md

[grammar] ~9-~9: There might be a mistake here.
Context: ...r ghost elements during drag operations. - DRAGGING: 1000 — Active dragging element. - `DRO...

(QB_NEW_EN)


[grammar] ~10-~10: There might be a mistake here.
Context: ...AGGING: 1000 — Active dragging element. - DROPPED`: 9999 — Element after being dropped. - ...

(QB_NEW_EN)


[grammar] ~11-~11: There might be a mistake here.
Context: ...ED: 9999 — Element after being dropped. - HEADER: 10 — Header bar. - THEME_TOGGLE`: 9 —...

(QB_NEW_EN)


[grammar] ~12-~12: There might be a mistake here.
Context: ...ng dropped. - HEADER: 10 — Header bar. - THEME_TOGGLE: 9 — Theme toggle button. - `CONTROL_PA...

(QB_NEW_EN)


[grammar] ~13-~13: There might be a mistake here.
Context: ...THEME_TOGGLE: 9 — Theme toggle button. - CONTROL_PANEL: 5 — Control panel UI. - `FREEMODE_TITL...

(QB_NEW_EN)


[grammar] ~14-~14: There might be a mistake here.
Context: ...- CONTROL_PANEL: 5 — Control panel UI. - FREEMODE_TITLE: 5 — Title in freemode. ## DEFAULTS D...

(QB_NEW_EN)


[grammar] ~21-~21: There might be a mistake here.
Context: ...5 — Default spacing between stack items. - STACK_SPACING_DEMO: 10 — Spacing for demo mode. - `STACK_D...

(QB_NEW_EN)


[grammar] ~22-~22: There might be a mistake here.
Context: ...ACING_DEMO: 10 — Spacing for demo mode. - STACK_DIRECTION`: 'horizontal' — Default stack direction...

(QB_NEW_EN)


[grammar] ~23-~23: There might be a mistake here.
Context: ... 'horizontal' — Default stack direction. - STACK_DIRECTION_VERTICAL: 'vertical' — Vertical stack direction....

(QB_NEW_EN)


[grammar] ~24-~24: There might be a mistake here.
Context: ...: 'vertical' — Vertical stack direction. - STACK_DIRECTION_HORIZONTAL: 'horizontal' — Horizontal stack direct...

(QB_NEW_EN)


[grammar] ~31-~31: There might be a mistake here.
Context: ...NE: 'allonsh-dropzone' — Dropzone area. - HIGHLIGHT`: 'allonsh-highlight' — Highlighted elem...

(QB_NEW_EN)


[grammar] ~32-~32: There might be a mistake here.
Context: ...llonsh-highlight' — Highlighted element. - DRAGGABLE: 'allonsh-draggable' — Draggable elemen...

(QB_NEW_EN)


[grammar] ~33-~33: There might be a mistake here.
Context: ...'allonsh-draggable' — Draggable element. - RESTRICTED: 'restricted' — Restricted area or elem...

(QB_NEW_EN)


[grammar] ~40-~40: There might be a mistake here.
Context: ...sition values: - RELATIVE: 'relative' - ABSOLUTE: 'absolute' ## CSS_CURSORS Cursor sty...

(QB_NEW_EN)


[grammar] ~47-~47: There might be a mistake here.
Context: ...and-drop interactions: - GRAB: 'grab' - GRABBING: 'grabbing' ## OPACITY Opacity values...

(QB_NEW_EN)


[grammar] ~54-~54: There might be a mistake here.
Context: ... '0.3' — Semi-transparent ghost element. - FULL: '1' — Fully opaque element. ## EVENTS...

(QB_NEW_EN)


[grammar] ~61-~61: There might be a mistake here.
Context: ...le: - DRAG_START: 'allonsh-dragstart' - DROP: 'allonsh-drop' - DRAG_ENTER: 'allons...

(QB_NEW_EN)


[grammar] ~62-~62: There might be a mistake here.
Context: ...onsh-dragstart' - DROP: 'allonsh-drop' - DRAG_ENTER: 'allonsh-dragenter' - DRAG_LEAVE: 'a...

(QB_NEW_EN)


[grammar] ~63-~63: There might be a mistake here.
Context: ...rop' - DRAG_ENTER: 'allonsh-dragenter' - DRAG_LEAVE: 'allonsh-dragleave' ## FLEX_DIRECTION...

(QB_NEW_EN)


[grammar] ~70-~70: There might be a mistake here.
Context: ...lexbox direction values: - ROW: 'row' - COLUMN: 'column' ## FLEX_WRAP Flexbox wrap m...

(QB_NEW_EN)


[grammar] ~83-~83: There might be a mistake here.
Context: ...splay property values: - FLEX: 'flex' - NONE: 'none' ## POINTER_EVENTS Pointer eve...

(QB_NEW_EN)


[grammar] ~90-~90: There might be a mistake here.
Context: ...NONE: 'none' — Disables pointer events. - AUTO`: 'auto' — Enables pointer events. ## U...

(QB_NEW_EN)

🔇 Additional comments (16)
src/constants.js (1)

1-69: Good centralization and naming—LGTM overall.

Clear groupings, sensible keys, and consistent naming across the codebase.

docs/constants.md (1)

21-26: If you split out STACK_DIRECTIONS, reflect it here.

Keep docs aligned with the enum vs defaults distinction.

test/allonsh.test.js (2)

154-166: Good switch to EVENTS.DROP.

Keeps tests resilient to future event-name changes.

Also applies to: 3-3


1-168: No stray hard-coded event names detected
I scanned the entire repository for raw allonsh-* strings. All matches are confined to the event constants (src/constants.js) and their references in documentation (README.md, docs/, CHANGELOG.md). There are no unexpected hard-coded custom events in the source code—no further changes needed.

src/allonsh.js (12)

1-13: LGTM: imports are clear and comprehensive.
All referenced constants appear to be used.


23-25: LGTM: defaults centralized via DEFAULTS.
This improves intent and consistency.


214-223: LGTM: stacking styles via constants.
Display, direction, gap, and wrap now self-document through enums.


306-310: LGTM: restoring opacity on pointer up.
Matches ghost behavior and avoids lingering dim state.


463-466: LGTM: pointer-events flip for accurate elementFromPoint hit-testing.
Prevents the dragged element from shadowing the target.


553-558: LGTM: normalized dropped state in free-drop path.
Absolute positioning and distinct z-index via constants are clear.


598-601: LGTM: restricted dropzone check via constants.
Simple and readable.


602-625: LGTM: state reset is thorough.
Cursor, z-index, highlight, and ghost cleanup handled; final nulling guards future drags.


638-646: LGTM: position reset uses constants.
Avoids style drift.


648-656: LGTM: highlight toggling via CSS_CLASSES.
Consistent with initialization.


690-702: Ensure removeDraggable() uses the same selector as addDraggable()
In removeDraggable, you’re re-querying draggables using only .${CSS_CLASSES.DRAGGABLE}, but the instance may have been initialized with a custom selector (this.draggableSelector). To keep your internal list in sync with user-defined draggables, mirror the union (or just use this.draggableSelector) rather than hard-coding the CSS class:

-      this.draggableElements = this.playAreaElement.querySelectorAll(
-        `.${CSS_CLASSES.DRAGGABLE}`
-      );
+      this.draggableElements = this.playAreaElement.querySelectorAll(
+        `${this.draggableSelector}, .${CSS_CLASSES.DRAGGABLE}`
+      );

Please verify that:

  • this.draggableSelector is indeed defined (typically in the constructor) and used when adding new draggables
  • The change won’t break existing behavior when no custom selector is provided

671-685: The proposed diff won’t work because this.draggableSelector doesn’t exist.
The class never saves the passed-in draggableSelector to an instance field, so using this.draggableSelector will be undefined. Before you can unify both selectors, you must:

• In the constructor, after validation, add:

this.draggableSelector = draggableSelector;

• In update(), if newOptions.draggableSelector is provided, reassign:

if (draggableSelector !== undefined) {
  this.draggableSelector = draggableSelector;
}

• Only then can you replace the hard-coded marker query in addDraggable() with something like:

this.draggableElements = this.playAreaElement.querySelectorAll(
  `.${this.draggableSelector}, .${CSS_CLASSES.DRAGGABLE}`
);

Alternatively—and more simply—avoid rebuilding the full list in addDraggable() altogether and instead append the newly made element to your existing collection (or maintain an array), letting update() remain the sole place that refreshes and binds all draggables.

Likely an incorrect or invalid review comment.

Comment on lines 84 to 105
import Allonsh from 'allonsh';
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',
restrictToDropzones: true,
enableStacking: true,
stackDirection: 'vertical',
stackSpacing: 10,
useGhostEffect: true,
});

// Listen for custom events
document.addEventListener(EVENTS.DRAG_START, (e) => {
// handle drag start
});
document.addEventListener(EVENTS.DROP, (e) => {
// handle drop
});
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Ensure allonsh/constants is exported by the package.

If not present in package.json exports, these import examples will break for consumers.

Run:


🏁 Script executed:

#!/bin/bash
# Verify package exports include a constants entry.
if [ -f package.json ]; then
  jq '.exports // .module // .main' package.json
  echo "---- constants entry ----"
  jq -r '.exports["./constants"] // empty' package.json
else
  echo "package.json not found in repo root"
fi

Length of output: 208


Add missing ./constants export in package.json

The current exports field is defined as a string ("src/allonsh.js") and does not include a subpath for constants. As a result, consumers will be unable to import from 'allonsh/constants'. Please update your package’s exports to expose the constants entry.

• In package.json, modify the exports section:

"exports": {
-  ".": "./src/allonsh.js"
+  ".": "./src/allonsh.js",
+  "./constants": "./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.

Suggested change
import Allonsh from 'allonsh';
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',
restrictToDropzones: true,
enableStacking: true,
stackDirection: 'vertical',
stackSpacing: 10,
useGhostEffect: true,
});
// Listen for custom events
document.addEventListener(EVENTS.DRAG_START, (e) => {
// handle drag start
});
document.addEventListener(EVENTS.DROP, (e) => {
// handle drop
});
```
// In your package.json
{
// … other fields …
"exports": {
".": "./src/allonsh.js",
"./constants": "./src/constants.js"
}
// … other fields …
}
🤖 Prompt for AI Agents
In docs/allonsh.md around lines 84 to 105, the example imports from
'allonsh/constants' but the package.json exports only the main bundle string and
doesn't expose a ./constants subpath; update package.json "exports" to add a
"./constants" entry that points to your constants build file (e.g.
"./src/constants.js" or the compiled "./dist/constants.js") so consumers can
import 'allonsh/constants' (e.g. add `"./constants": {"import":
"./dist/constants.mjs","require": "./dist/constants.cjs"}` or a simple
`"./constants": "./dist/constants.js"` depending on your build).

@p55d2k
Copy link
Author

p55d2k commented Aug 27, 2025

Hi, I believe there's a problem with github adding a comment to the PR, @satnam72 would you mind taking a look? Not really sure what's going on here, thanks!

@satnam72 satnam72 changed the base branch from main to dev August 27, 2025 15:10
@satnam72
Copy link
Owner

Hi @p55d2k, thank you for the contribution. Please ignore the error, Add comment to PR is still in progress. The changes will be merged to dev branch and once everything is ready, it will be merged to main.

@satnam72 satnam72 merged commit 4e69f23 into satnam72:dev Aug 27, 2025
2 of 3 checks passed
@p55d2k p55d2k deleted the fix/constants-refactor branch August 28, 2025 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants