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

(2.2) feat: Add Feedback Form UI Branding logo #4357

Open
wants to merge 6 commits into
base: antonis/3859-newCaptureFeedbackAPI-Form
Choose a base branch
from
+49 −5
Open

(2.2) feat: Add Feedback Form UI Branding logo #4357

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6953a3
Adds sentry branding (png logo)
antonis Dec 11, 2024
d22d660
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/3…
antonis Dec 12, 2024
c02d84f
Merge branch 'antonis/3859-newCaptureFeedbackAPI-Form' into antonis/3…
antonis Dec 13, 2024
61e4d4d
Use network resource to avoid packaging issues
antonis Dec 13, 2024
fbb402b
Adds test
antonis Dec 13, 2024
bb79879
Add default value in the doc comment
antonis Dec 13, 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
11 changes: 10 additions & 1 deletion packages/core/src/js/feedback/FeedbackForm.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ const defaultStyles: FeedbackFormStyles = {
fontSize: 24,
fontWeight: 'bold',
marginBottom: 20,
textAlign: 'center',
textAlign: 'left',
flex: 1,
},
label: {
marginBottom: 4,
Expand Down Expand Up @@ -49,6 +50,14 @@ const defaultStyles: FeedbackFormStyles = {
color: '#6a1b9a',
fontSize: 16,
},
titleContainer: {
flexDirection: 'row',
width: '100%',
},
sentryLogo: {
width: 40,
height: 40,
},
};

export default defaultStyles;
13 changes: 11 additions & 2 deletions packages/core/src/js/feedback/FeedbackForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { captureFeedback, lastEventId } from '@sentry/core';
import type { SendFeedbackParams } from '@sentry/types';
import * as React from 'react';
import type { KeyboardTypeOptions } from 'react-native';
import { Alert, Text, TextInput, TouchableOpacity, View } from 'react-native';
import { Alert, Image, Text, TextInput, TouchableOpacity, View } from 'react-native';

import { defaultConfiguration } from './defaults';
import defaultStyles from './FeedbackForm.styles';
Expand Down Expand Up @@ -80,7 +80,16 @@ export class FeedbackForm extends React.Component<FeedbackFormProps, FeedbackFor

return (
<View style={styles.container}>
<Text style={styles.title}>{text.formTitle}</Text>
<View style={styles.titleContainer}>
<Text style={styles.title}>{text.formTitle}</Text>
{config.showBranding && (
<Image
Copy link
Collaborator

@lucas-zimerman lucas-zimerman Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we use an svg image?

from here: https://sentry.io/branding/

we could have an SVG image, like this one:

<svg class="css-lfbo6j e1igk8x04" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 50 44" width="400" height="352" style=""><path d="M29,2.26a4.67,4.67,0,0,0-8,0L14.42,13.53A32.21,32.21,0,0,1,32.17,40.19H27.55A27.68,27.68,0,0,0,12.09,17.47L6,28a15.92,15.92,0,0,1,9.23,12.17H4.62A.76.76,0,0,1,4,39.06l2.94-5a10.74,10.74,0,0,0-3.36-1.9l-2.91,5a4.54,4.54,0,0,0,1.69,6.24A4.66,4.66,0,0,0,4.62,44H19.15a19.4,19.4,0,0,0-8-17.31l2.31-4A23.87,23.87,0,0,1,23.76,44H36.07a35.88,35.88,0,0,0-16.41-31.8l4.67-8a.77.77,0,0,1,1.05-.27c.53.29,20.29,34.77,20.66,35.17a.76.76,0,0,1-.68,1.13H40.6q.09,1.91,0,3.81h4.78A4.59,4.59,0,0,0,50,39.43a4.49,4.49,0,0,0-.62-2.28Z" fill="#362d59"></path></svg>

I like the idea of using the svg icon because it's easy to customize the logo color, so if a user decides to use a dark theme, the Sentry logo will not be hidden (by that we would need to allow them to change the sentry color or maybe some logic to know if the background color is dark or bright)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point. I defaulted to PNG due to the discussion with @krystofwoldrich on the form PR.
I think that using an svg directly might require a library which I'm not sure we want to import in core 🤔
Maybe I can try using a JSX with a convertor like https://www.svgviewer.dev/svg-to-react-jsx. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

We can't use SVG, because there is no out of the box support and we can't add dependency just to render our logo and vendoring a library would also not work due to size and potential build issues.

https://www.svgviewer.dev/svg-to-react-jsx would work only for web

source={{ uri: 'https://sentry-brand.storage.googleapis.com/sentry-glyph-black.png' }}
Copy link
Member

Choose a reason for hiding this comment

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

Let's include logo locally.

I'm not sure who in Sentry owns this storage but it could be gone any time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point @krystofwoldrich 👍
I actually tried this (61e4d4d) but run into errors with packaging. I'll revisit my approach.

style={styles.sentryLogo}
testID='sentry-logo'
/>
)}
</View>

{config.showName && (
<>
Expand Down
18 changes: 17 additions & 1 deletion packages/core/src/js/feedback/FeedbackForm.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import type { TextStyle, ViewStyle } from 'react-native';
import type { ImageStyle, TextStyle, ViewStyle } from 'react-native';

/**
* The props for the feedback form
*/
export interface FeedbackFormProps extends FeedbackGeneralConfiguration, FeedbackTextConfiguration, FeedbackCallbacks {
styles?: FeedbackFormStyles;
}
Expand All @@ -8,6 +11,11 @@ export interface FeedbackFormProps extends FeedbackGeneralConfiguration, Feedbac
* General feedback configuration
*/
export interface FeedbackGeneralConfiguration {
/**
* Show the Sentry branding
*/
antonis marked this conversation as resolved.
Show resolved Hide resolved
showBranding?: boolean;

/**
* Should the email field be required?
*/
Expand Down Expand Up @@ -123,6 +131,9 @@ export interface FeedbackCallbacks {
onFormClose?: () => void;
}

/**
* The styles for the feedback form
*/
export interface FeedbackFormStyles {
container?: ViewStyle;
title?: TextStyle;
Expand All @@ -133,8 +144,13 @@ export interface FeedbackFormStyles {
submitText?: TextStyle;
cancelButton?: ViewStyle;
cancelText?: TextStyle;
titleContainer?: ViewStyle;
sentryLogo?: ImageStyle;
}

/**
* The state of the feedback form
*/
export interface FeedbackFormState {
isVisible: boolean;
name: string;
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/js/feedback/defaults.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export const defaultConfiguration: Partial<FeedbackFormProps> = {
},

// FeedbackGeneralConfiguration
showBranding: true,
isEmailRequired: false,
isNameRequired: false,
showEmail: true,
Expand Down
9 changes: 8 additions & 1 deletion packages/core/test/feedback/FeedbackForm.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,10 @@ describe('FeedbackForm', () => {
});

it('renders correctly', () => {
const { getByPlaceholderText, getByText } = render(<FeedbackForm {...defaultProps} />);
const { getByPlaceholderText, getByText, getByTestId } = render(<FeedbackForm {...defaultProps} />);

expect(getByText(defaultProps.formTitle)).toBeTruthy();
expect(getByTestId('sentry-logo')).toBeTruthy(); // default showBranding is true
expect(getByText(defaultProps.nameLabel)).toBeTruthy();
expect(getByPlaceholderText(defaultProps.namePlaceholder)).toBeTruthy();
expect(getByText(defaultProps.emailLabel)).toBeTruthy();
Expand All @@ -58,6 +59,12 @@ describe('FeedbackForm', () => {
expect(getByText(defaultProps.cancelButtonLabel)).toBeTruthy();
});

it('does not render the sentry logo when showBranding is false', () => {
const { queryByTestId } = render(<FeedbackForm {...defaultProps} showBranding={false} />);

expect(queryByTestId('sentry-logo')).toBeNull();
});

it('name and email are prefilled when sentry user is set', () => {
const { getByPlaceholderText } = render(<FeedbackForm {...defaultProps} />);

Expand Down
Loading