-
Notifications
You must be signed in to change notification settings - Fork 140
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
Make feedback tab survey display responsive #1704
base: main
Are you sure you want to change the base?
Conversation
The problem here is the `left` style applied to the element. This overrides with the styles applied in `surveys-utils.tsx`. Since the styles applied there are already responsive, removing this fixes the issue. The styles in `surveys-utils.tsx` are also more respectful of the survey's appearance configuration (left, center, right), so this actually fixes another unreported issue where the survey doesn't appear in the true center of the page.
The 30px tucks the survey right in the bottom right corner of the page when it's presented as a popover. But overlaps with the feedback button when presented in that manner. I'm opting to solve this by adding additional clearance in both configurations in favor of simplicity. It could be handled conditionally, but that would require some more logic. The "more logic" is more than I originally thought because - the way things are today - a user can configure a popover survey with a `appearance.widgetType` of tab. So properly handling this requires either adding validation to make that impossible or passing the whole survey - not just `SurveyAppearance` into this `style` utility function. Passing on that for now although there's a case for it.
@bvosk is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
@@ -701,7 +701,6 @@ export function FeedbackWidget({ | |||
}): JSX.Element { | |||
const [showSurvey, setShowSurvey] = useState(false) | |||
const [styleOverrides, setStyle] = useState({}) | |||
const widgetRef = useRef<HTMLDivElement>(null) |
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.
I ended up removing this as it's no longer needed. It muddies up the diff quite a bit, but have a look at the individual commits if you'd like to separate this change from the rest.
right: 'right: 30px;', | ||
right: 'right: 60px;', |
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.
Repeating my commit message here for discussion. This wasn't strictly necessary to fix the reported issue, but I believe this fix created this problem of overlap with the feedback button. Perhaps fixing this overlap motivated the style override in the first place. Not sure.
The 30px tucks the survey right in the bottom right corner of the page when it's presented as a popover. But overlaps with the feedback button when presented in that manner.
I'm opting to solve this by adding additional clearance in both configurations in favor of simplicity. It could be handled conditionally, but that would require some more logic.
The "more logic" is more than I originally thought because - the way things are today - a user can configure a popover survey with a
appearance.widgetType
of tab. So properly handling this requires either adding validation to make that impossible or passing the whole survey - not justSurveyAppearance
into thisstyle
utility function. Passing on that for now although there's a case for it.
Here's how it looks with the unchanged right: 30px
style.
Changes
Closes PostHog/posthog#27982
Before
After
Discussion
Ultimately the issue here is that there are some styles in
src/extensions/surveys.tsx
that override the ones insrc/extensions/surveys/surveys-utils.tsx
. So this was just a matter of removing the override causing the problem.In addition to the lack of responsive behavior, this same style was causing some positioning to be off on some device widths. Here's an example of that with the
center
position:Checklist