fix(feedback): Improve CSS theme variable names and layout #11964
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
ryan953
commented
May 9, 2024
billyvg
reviewed
May 9, 2024
Comment on lines
+33
to
+35
| --font-family: system-ui, 'Helvetica Neue', Arial, sans-serif; | ||
| --font-size: 14px; | ||
| --z-index: 100000; |
There was a problem hiding this comment.
Are we removing this as available customization options from constructor?
There was a problem hiding this comment.
ya, it wasn't on docs.sentry.io, and font stuff wasn't possible to override. So i pulled it out, but it's easy to add back.
I think i'd like a general-theme section in there that's neither dark or light mode. In my testing i screwed up a couple times by setting something like themeLight: { foreground: 'red' } then rendering in dark mode i was confused why i couldn't see my color change. It wouldn't be great to have to specify font in both places, or only in one place and have it copied around imo.
So i was thinking, shorter term at least, we lean into css variables more in the docs, so that's the first stop for people. And that's where everything is available. https://github.com/getsentry/sentry-docs/pull/9961/files
billyvg
approved these changes
May 9, 2024
|
we're missing the outline focus changes :( |
andreiborza
pushed a commit
that referenced
this pull request
May 16, 2024
This updates the CSS theme, variable names and also how the css works.
The widget is better able to handle cases like:
- short desktop window (scrolls form)
- phone in portrait (full width if the screen is 600px or less)
- phone in landscape (scrolls form)
- when screenshots are in use, stick to screen edges
I also made it, I think, easier to position the button (aka trigger, aka
actor) around the screen. You can override in css `--page-margin: 16px`
to dictate the distance from the edge, and override `--inset: auto 0 0
auto;` to position the button in the different corners of the screen.
For example, you can put the trigger button on the edge of the screen
with something like:
```css
#sentry-feedback {
--actor-inset: auto 0 calc(50% - var(--header-height)) auto;
}
```
Docs are updated in getsentry/sentry-docs#9961
to reflect the changed config/variable names too.
Some samples of different situations:
| Desc | Img |
| --- | --- |
| Normal |
| Short Window | 
| Screenshot editor |
| Mobile landscape | 
| Mobile portrait | 
| Override Color | 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the CSS theme, variable names and also how the css works.
The widget is better able to handle cases like:
I also made it, I think, easier to position the button (aka trigger, aka actor) around the screen. You can override in css
--page-margin: 16pxto dictate the distance from the edge, and override--inset: auto 0 0 auto;to position the button in the different corners of the screen. For example, you can put the trigger button on the edge of the screen with something like:Docs are updated in getsentry/sentry-docs#9961 to reflect the changed config/variable names too.
Some samples of different situations: