Skip to content

Conversation

@Fellan-91
Copy link
Collaborator

@Fellan-91 Fellan-91 commented Oct 24, 2025

Summary

This PR adds a new beforeLoadNewItems event hook that fires before loading new items in sort, filter, and pagination interactions. This enhancement provides better extensibility for Pro modules to intercept and customize navigation behavior.

Changes:

Event Data Structure:

{
  url: string,           // The URL to load
  removeExisting: boolean, // Whether to replace or append items
  cancelled: false       // Set to true to prevent default behavior
}

Use Case:
Pro modules can now listen to this event and set eventData.cancelled = true to handle navigation themselves, enabling custom routing, animations, or other advanced features without modifying core code.

🤖 Generated with Claude Code

Fellan-91 and others added 2 commits October 24, 2025 15:34
This change introduces a new 'beforeLoadNewItems' event that fires before loading new items in sort, filter, and pagination interactions. This allows Pro modules to intercept navigation by setting eventData.cancelled = true, enabling custom navigation handling.

The event is emitted in three key places:
- Sort click handler
- Filter/sort select change handler
- Pagination click handler

Each event includes url, removeExisting, and cancelled properties.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@claude
Copy link

claude bot commented Oct 24, 2025

Code Review

I've reviewed the changes in this PR. Overall, this is a well-designed enhancement that adds a clean extension point for Pro modules. Here are my findings:

✅ Strengths

  1. Consistent implementation - The beforeLoadNewItems event is added consistently across all three interaction points (sort, filter select, pagination)
  2. Clear event data structure - The eventData object has a clean, predictable structure with url, removeExisting, and cancelled properties
  3. Good documentation - Inline comments clearly explain the purpose and usage pattern
  4. Backward compatible - Existing functionality is preserved; the event is purely additive

🔍 Issues Found

1. Missing event hook in filter click handler (assets/js/main.js:547-562)

The filter click handler is missing the beforeLoadNewItems event emission. This creates an inconsistency:

  • ✅ Sort click handler - has event (line 577)
  • Filter click handler - MISSING event (line 547-562)
  • ✅ Filter/sort select change - has event (line 615)
  • ✅ Pagination click - has event (line 685)

This should be added for consistency:

self.$filter.on(
    `click${evp}`,
    '.vp-filter .vp-filter__item a',
    function (e) {
        e.preventDefault();
        const $this = $(this);
        const href = $this.attr('href');

        // Emit event BEFORE loadNewItems - allows Pro modules to intercept
        const eventData = {
            url: href,
            removeExisting: true,
            cancelled: false,
        };
        self.emitEvent('beforeLoadNewItems', [eventData]);

        if (eventData.cancelled) {
            return; // Pro module will handle navigation
        }

        if (!self.loading) {
            $this
                .closest('.vp-filter__item')
                .addClass('vp-filter__item-active')
                .siblings()
                .removeClass('vp-filter__item-active');
        }
        self.loadNewItems(href, true);
    }
);

2. Missing event hook in category link handler (assets/js/main.js:698-706)

The category link click handler also triggers loadNewItems but doesn't emit the event. For completeness, it should also include the hook:

self.$item.on(
    `click${evp}`,
    '.vp-portfolio__items .vp-portfolio__item-meta-category a',
    function (e) {
        e.preventDefault();
        e.stopPropagation();
        const url = $(this).attr('href');

        // Emit event BEFORE loadNewItems
        const eventData = {
            url,
            removeExisting: true,
            cancelled: false,
        };
        self.emitEvent('beforeLoadNewItems', [eventData]);

        if (eventData.cancelled) {
            return;
        }

        self.loadNewItems(url, true);
    }
);

📝 Recommendations

  1. Add the event to all navigation handlers - Ensure every code path that calls loadNewItems() emits the beforeLoadNewItems event first
  2. Update the build - Remember to run npm run build (not build:prod) after making JavaScript changes
  3. Consider event data modification - Document whether Pro modules can modify url or removeExisting properties, or if they should only use cancelled

Verdict: Approve with changes requested. The implementation is solid but needs the missing event hooks added for consistency and completeness.

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