-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Issue 34961 GraphQL Logo seems to be incorrect #34974
fix: Issue 34961 GraphQL Logo seems to be incorrect #34974
Conversation
WalkthroughThis update addresses the incorrect display of the GraphQL logo in the integration editor. The change replaces the logo used for GraphQL API integration from the current Changes
Sequence Diagram(s)None needed for this simple change. Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/pages/Editor/IntegrationEditor/NewApi.tsx (2 hunks)
Additional comments not posted (2)
app/client/src/pages/Editor/IntegrationEditor/NewApi.tsx (2)
10-10
: Correct import for GraphQL logo.The addition of the
GraphQLLogo
import from the correct path is a necessary step to resolve the issue with the incorrect logo display. This change is directly aligned with the PR's objective to differentiate the GraphQL logo from the RestAPI logo. Well done on ensuring the path matches the project's structure.
270-270
: Updated image source to use the correct logo.You have correctly updated the image source for the GraphQL API card to use the newly imported
GraphQLLogo
. This change ensures that the correct logo is displayed, resolving the cosmetic issue reported. It's important to visually confirm this change in the UI to ensure that the logo appears as expected.Verification successful
Let's verify the import and usage of
GraphQLLogo
in theNewApi.tsx
file to ensure it is correctly implemented.
Updated image source to use the correct logo.
You have correctly updated the image source for the GraphQL API card to use the newly imported
GraphQLLogo
. This change ensures that the correct logo is displayed, resolving the cosmetic issue reported. It's important to visually confirm this change in the UI to ensure that the logo appears as expected.
- The import statement for
GraphQLLogo
is correctly set to"assets/images/Graphql-logo.svg"
.- The
GraphQLLogo
is used in the component withsrc={GraphQLLogo}
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct display of the GraphQL logo in the UI. # Test: Check the rendered HTML for the presence of the GraphQL logo image. echo "Manual verification required: Please ensure the GraphQL logo is correctly displayed in the UI."Length of output: 198
Script:
#!/bin/bash # Verify the import statement for GraphQLLogo rg 'import .*GraphQLLogo.* from' app/client/src/pages/Editor/IntegrationEditor/NewApi.tsx # Verify the usage of GraphQLLogo in the component rg 'GraphQLLogo' -A 5 app/client/src/pages/Editor/IntegrationEditor/NewApi.tsxLength of output: 826
Hi @ayushpahwa . Please review. |
Hi @ayushpahwa , please review this PR. |
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10053916997. |
Deploy-Preview-URL: https://ce-34974.dp.appsmith.com |
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. Thanks for your contribution!
@ayushpahwa do we need any tests for this? |
@NilanshBansal for UI changes like this, snapshot tests might be relevant but they are not really maintainable. I have done manual test on the DP generated however. Lemme know if there's any other suggestions/reservations. |
Thanks @ayushpahwa for the confirmation! Looks good to me. Let's go ahead and get this merged in that case. |
All automation tests passed, okay to merge. |
Description
Issue: The GRAPHQL plugin logo is shown same as RestAPI plugin.
Fix: Imported the right logo for graphql and used the right image in the img tag
Fixes #34961
Automation
/ok-to-test tags="@tag.Sanity @tag.IDE"
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit