-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add example image to Sentry iOS user feedback page #14503
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.
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
Add example image to Sentry iOS user feedback page #14503
Conversation
Co-authored-by: bruno <[email protected]>
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bundle ReportChanges will decrease total bundle size by 21.23kB (-0.09%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-server-cjsAssets Changed:
Files in
Files in
App Routes Affected:
view changes for bundle: sentry-docs-client-array-pushAssets Changed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but the linter complaints.
@bruno-garcia looks like this is causing a 404 issue. Check it out and try to push a commit through again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…r-feedback-page-09c2
src/components/docImage/index.tsx
Outdated
height={height} | ||
isManualDimensions={isManual} | ||
alt={props.alt ?? ''} | ||
{...props} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, this should be mostly fine. There is one small potential issue that we can safeguard. It's more from the original implementation but it caught my eye with this additional move. The prop spreading should just come before the height and width attributes. There's a world in which the string value MDX props would override the parsed Numbers. If that gets down to the Next.js <Image>
, it'd be a problem. But if we just do it earlier, the parse will win when we need it to.
src/components/docImage/index.tsx
Outdated
width={width} | ||
height={height} | ||
isManualDimensions={isManual} | ||
alt={props.alt ?? ''} | ||
{...props} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
width={width} | |
height={height} | |
isManualDimensions={isManual} | |
alt={props.alt ?? ''} | |
{...props} | |
isManualDimensions={isManual} | |
alt={props.alt ?? ''} | |
{...props} | |
width={width} | |
height={height} |
I take whatever you say for truth 😅 you mean like this^ right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! It is a nuanced and maybe unlikely scenario, but safety first🥇 Let me grab the whole component for completeness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 3e50084
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, have a few comments but not blockers.
We may wanna open-source this as a separate package if there's nothing like thos out there?
width: width != null ? `${width}px` : 'auto', | ||
height: height != null ? `${height}px` : 'auto', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you may wanna allow different units for the future too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point! I will skip it for now though, can always add it later. Will leave a todo
…r-feedback-page-09c2
The review has been addressed, we can follow up with another PR if there's still something off with the props order
Note
Adds alt-text-based image sizing via a new remark plugin, updates image components to prefer manual dimensions, and inserts an example widget image in iOS user feedback docs.
src/components/docImage
now prefers explicitwidth
/height
props over URL hash dimensions and passesisManualDimensions
to the lightbox.src/components/imageLightbox
respectsisManualDimensions
, applying only provided dimensions (other axis auto) for inline images.remark-image-resize.js
and wire it insrc/mdx.ts
to parse size hints from image ALT text (e.g.,=300x200
) and setwidth
/height
attributes.docs/platforms/apple/common/user-feedback/index.mdx
.Written by Cursor Bugbot for commit 3e50084. This will update automatically on new commits. Configure here.