-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(platform): preview project name on top of DSN #13041
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
Conversation
also fixes a "copied" feedback z-index issue closes #13015
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bundle ReportChanges will increase total bundle size by 2.53kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-server-cjsAssets Changed:
Files in
App Routes Affected:
view changes for bundle: sentry-docs-client-array-pushAssets Changed:
Files in
|
|
@a-hariti Thanks for the quick implementation! How should we deal with the project preview overlaying code? One idea that popped to my mind would be reducing the opacity when the cursor is on top of the preview, so that it is possible to view what's below the code, but not sure about that. https://sentry-docs-git-dsn-preview.sentry.dev/platforms/apple/guides/ios/
|
|
The ideal solution might be to track the user's gaze with AI and move the popup out of their eyesight 😂 |
|
I came up with this hack to add some margin to lines with clickable "keywords", it doesn't discriminate between DSN's or other things, but the worst case scenario is having an apparent extra line of whitespace, so it should do @philprime
|
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.
Found a typo, added suggestions.
As my background is more mobile UI than web UI development, it would be great if @chargome could do a proper review on the implementation.
also rename styles file to be an actual TS file with the confusing .css.ts extention
|
@a-hariti Am I missing something or is the preview for DSNs not in there anymore? |
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.
Code looks fine but it does not seem to pick up all usage cases:
https://sentry-docs-git-dsn-preview.sentry.dev/platforms/javascript/guides/node/

https://sentry-docs-git-dsn-preview.sentry.dev/platforms/apple/guides/ios/

https://sentry-docs-git-dsn-preview.sentry.dev/platforms/dart/guides/flutter/

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.
Can we make the tooltip not to be picked up from selection?
I selected the text to copy it (instead of using the copy button) and it copied like this:
@import Sentry;
(BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
[SentrySDK startWithConfigureOptions:^(SentryOptions *options) {
options.dsn = @"https://[email protected]/0example-org / example-project";
options.debug = YES; // Enabled debug when first installing is always helpful
// Adds IP for users.
// For more information, visit: https://docs.sentry.io/platforms/apple/data-management/data-collected/
options.sendDefaultPii = YES
}];
return YES;
}It seems to append the example-org / example-project to the DSN, while visually it is not selected.
This will cause UX issues with users who prefer to select code to copy it.
…ck copy or / Cmd+C
|
Good catch @philprime, it required some gymnastics, but I believe it's good to go now :) |
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.
@a-hariti could we only display this when the user is logged in? Not really useful when the example project is shown I think 🤔
|
@a-hariti - what's the status of this one? |
|
I can take a look yes, otherwise @sergical is the man to ping now! |
| document.addEventListener('selectionchange', handleSelectionChange); | ||
| return () => { | ||
| document.removeEventListener('selectionchange', handleSelectionChange); | ||
| }; |
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.
Bug: Selection Listener Fails with Dynamic Content
The selectionchange listener has a few issues. It captures .no-copy elements only once, missing dynamic additions, causing them to remain visible during selection. Restoring elements unconditionally sets display: 'inline', which can break their original layout. This hide/show behavior is visually jarring, and multiple CodeBlock instances add redundant global listeners.
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
@sergical just leaving this open for you to take a look – feel free to merge in after!
| border: none; | ||
| color: var(--white); | ||
| transition: opacity 150ms; | ||
| z-index: 10000; |
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.
Not sure if this is really necessary, but should not interfere with anything else I think





also fixes a "copied" feedback z-index issue
closes #13015