-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: restored Add logo button #2537
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
Thank you for following the naming conventions for pull request titles! 🙏 |
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.
@Dhruwang thanks for the PR :-) A few points I noticed from looking at the code:
apps/web/app/(app)/environments/[environmentId]/settings/lookandfeel/components/EditLogo.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/(app)/environments/[environmentId]/settings/lookandfeel/components/EditLogo.tsx
Outdated
Show resolved
Hide resolved
@@ -43,6 +43,11 @@ export const ClientLogo = ({ environmentId, product, previewSurvey = false }: Cl | |||
) : ( | |||
<Link | |||
href={`/environments/${environmentId}/settings/lookandfeel`} | |||
onClick={(e) => { | |||
if (!environmentId) { |
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.
Should we show the link in the first place if the environmentId is not set?
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.
Here link is actually the Add logo button
![Screenshot 2024-04-29 at 12 41 52 PM](https://private-user-images.githubusercontent.com/67850763/326354455-ad9d79b6-8524-4b35-bd48-e302bc8eea23.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQzMTkzMDksIm5iZiI6MTcyNDMxOTAwOSwicGF0aCI6Ii82Nzg1MDc2My8zMjYzNTQ0NTUtYWQ5ZDc5YjYtODUyNC00YjM1LWJkNDgtZTMwMmJjOGVlYTIzLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIyVDA5MzAwOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTIxOTk4YWFlYjc1ZjliZWY0NmE0NjUzZDEwMTRjYjExMmUxZGE5OGIzOTJlZDI1MjgzMjEyNDI2YTczMTljY2UmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.olu2bs-cCZ42jhrvSCFhp-26O-6wWxJxw75yxQ6Z2-I)
And when environmentId is not present, which is the case on look and feel page preview, there it would be better that we still show the Add Logo
Link which on click scrolls down to Add Logo
settings. What do you think ?🤔
What does this PR do?
Fixes https://github.com/formbricks/internal/issues/120
![Screenshot 2024-04-26 at 11 44 45 AM](https://private-user-images.githubusercontent.com/67850763/325876343-c622a077-8e73-4cb7-8ef2-dd1fb0199f84.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQzMTkzMDksIm5iZiI6MTcyNDMxOTAwOSwicGF0aCI6Ii82Nzg1MDc2My8zMjU4NzYzNDMtYzYyMmEwNzctOGU3My00Y2I3LThlZjItZGQxZmIwMTk5Zjg0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODIyVDA5MzAwOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTAxOGI1MzVkOGUxMmQ1NzdiMzdlNzlkOTRhNjIzMjA2NDdkN2U1NGE1MTljMmQyODYxZDljN2E1YTgzNzc4YTMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.Z1lOzQTH--VdmxqR17D7y87Lt-qs4d3N_EaB6QEDXLk)
Additionally, replaced window.confirm modal with our
DeleteDialog
, when removing a logoHow should this be tested?
Test survey Preview in survey Editor and on look and feel page
Checklist
Required
pnpm build
console.logs
git pull origin main
Appreciated