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.

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

feat: update page titles for dashboards and alerts #6706

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nishantharcot
Copy link

@nishantharcot nishantharcot commented Dec 24, 2024

Summary

The titles of dashboards and alerts are updated to their names.

Screenshots

Alert-Title Dashboard-Title

Important

Update document.title in AlertDetails.tsx and DashboardPage.tsx to reflect alert and dashboard names.

  • Behavior:
    • Update document.title in AlertDetails.tsx to the alert name from alertDetailsResponse.
    • Update document.title in DashboardPage.tsx to the dashboard name from dashboardResponse.
  • Files Affected:
    • AlertDetails.tsx
    • DashboardPage.tsx

This description was created by Ellipsis for 86a74a6. It will automatically update as commits are pushed.

Copy link

welcome bot commented Dec 24, 2024

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

@CLAassistant
Copy link

CLAassistant commented Dec 24, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 86a74a6 in 36 seconds

More details
  • Looked at 29 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/pages/AlertDetails/AlertDetails.tsx:84
  • Draft comment:
    Consider extracting the document title update logic into a reusable function to adhere to the DRY principle. This logic is also present in DashboardPage.tsx.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The document title update logic is repeated in both files. It would be better to extract this logic into a reusable function to adhere to the DRY principle.
2. frontend/src/pages/NewDashboard/DashboardPage.tsx:21
  • Draft comment:
    Avoid using inline styles for setting document titles. Consider using a React effect hook to manage side effects like updating the document title.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_UBLf4YItn1uZTHvi


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -80,6 +80,9 @@ function AlertDetails(): JSX.Element {
alertDetailsResponse,
} = useGetAlertRuleDetails();

const alertTitle = alertDetailsResponse?.payload?.data.alert;
document.title = alertTitle || document.title;
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using inline styles for setting document titles. Consider using a React effect hook to manage side effects like updating the document title.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the titles in useEffect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants