Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(feedback): Add missing h import in ScreenshotEditor (#12713) #12784

Merged
merged 2 commits into from
Jul 8, 2024
Merged

fix(feedback): Add missing h import in ScreenshotEditor (#12713) #12784

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd415e2
fix(feedback): Add missing `h` import in `ScreenshotEditor` (#12713)
andreiborza Jul 5, 2024
26d19d9
Merge branch 'develop' into ab/fix-feedback-screenshot-button
ryan953 Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/feedback/src/screenshot/components/ScreenshotEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* eslint-disable max-lines */
import type { FeedbackInternalOptions, FeedbackModalIntegration } from '@sentry/types';
import type { ComponentType, VNode, h as hType } from 'preact';
// biome-ignore lint/nursery/noUnusedImports: reason
import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars
Comment on lines +4 to +5
Copy link
Member

Choose a reason for hiding this comment

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

can we add a reason/additional comment as to why we need this ignore? It looks like h is destructured but unused further below which doesn't look like an unused import on first glance 🤔 is this a biome bug?

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, I'm off here, this shouldn't require an import 😅
So I guess there's some side effect we need from importing h?

Copy link
Member Author

@andreiborza andreiborza Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not too familiar with preact, but I'm guessing it's a similar factory to JSX createElement? Just taking a wild guess.

This has been explicitly removed in favor of injecting h in #12535 but at that point it seems to be too late to do.

Maybe we should completely revert the injection too, but I'm waiting on @ryan953 for confirmation before going ahead here.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh alright sounds good to me! Sorry, I don't have much context around feedback or Preact and was a bit confused but makes sense!

Copy link
Member

Choose a reason for hiding this comment

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

yes h is needed because it's a similar factory to JSX createElement, it's needed anytime JSX syntax is used

import type * as Hooks from 'preact/hooks';
import { DOCUMENT, WINDOW } from '../../constants';
import { createScreenshotInputStyles } from './ScreenshotInput.css';
Expand Down
Loading