Skip to content

Conversation

@leandrowilliam
Copy link
Contributor

@leandrowilliam leandrowilliam commented Sep 22, 2025

The 'Open in New Tab' button was not working because it was looking for an iframe with class 'preview', but the UI revamp changed it to 'preview-iframe'. This updates the JavaScript selector to match.

  • Update to in open-preview click handler
  • Update to in refreshPreview function

Fixes the bug where clicking 'Open in New Tab' opened a blank page.

Closing issues: #1452

Before After
CleanShot 2025-09-22 at 12 58 36 CleanShot 2025-09-22 at 12 56 15

The 'Open in New Tab' button was not working because it was looking
for an iframe with class 'preview', but the UI revamp changed it to
'preview-iframe'. This updates the JavaScript selector to match.

- Update  to  in open-preview click handler
- Update  to  in refreshPreview function

Fixes the bug where clicking 'Open in New Tab' opened a blank page.
@leandrowilliam
Copy link
Contributor Author

@ricardo-devis-agullo I was tempted to write a unit test like this in registry-ui.js:

    it('should have matching iframe element and JavaScript selector for preview functionality', () => {
      const hasIframeElement = result.includes('class="preview-iframe"');
      const hasMatchingJavaScript = result.includes("$('.preview-iframe')");
      
      expect(hasIframeElement).to.be.true;
      expect(hasMatchingJavaScript).to.be.true;
    });

However, I feel this test would be quite specific and fragile, testing implementation details (JS/HTML/CSS class matching) rather than the actual functionality. I'm not sure if it's worthwhile.
To properly test the "Open in New Tab" functionality, we'd need browser automation (like Playwright or Puppeteer) to verify that clicking the button actually opens a new tab with the correct URL. I looked through the codebase but couldn't find any existing browser testing setup, and I'm not sure if adding such infrastructure would be appropriate for this simple bug fix.
What do you think? Should I add the test above, look into setting up browser testing, or keep this PR focused just on the bug fix?

@ricardo-devis-agullo
Copy link
Collaborator

thanks for the fix! I'd say we can merge this. Doing e2e tests to test the whole UI is nice but is a bigger story than this fix. There is technically a new file in preview/uiPreview.ts that you can run with Bun that starts a mocked local registry that you could attach playwright too, but its something that I might take on myself so no worries!

@leandrowilliam
Copy link
Contributor Author

Cool, sounds good!

@ricardo-devis-agullo ricardo-devis-agullo merged commit b4ad72f into opencomponents:master Sep 22, 2025
3 checks passed
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