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

Jump to bottom

fix:default title of the chart should be updated to the graphed value… #8602

Merged
merged 3 commits into from
May 20, 2024
Merged

fix:default title of the chart should be updated to the graphed value… #8602

merged 3 commits into from
May 20, 2024

Conversation

rajeshj11
Copy link
Contributor

@rajeshj11 rajeshj11 commented May 17, 2024 •
edited

fixes: #8579
/claim #8579

Summary

case 1: If the graph is created/edited without any title it will pick from function type and metric. [functionType(metric)]
case 2: If the user wants to give a proper name to it, they can fill in the title of their choice. The user's title will be given a higher priority irrespective of the updation of function and metric.

note: If the user wants the system to pick the title. Make the title empty. It will automatically pick the title based on function type and metric.

How did you test this change?

https://www.loom.com/share/9fd9a1e000d84c5098afe7330f0dc6e5

Are there any deployment considerations?

No

Does this work require review from our design team?

No

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 17, 2024 •
edited

⚠️ No Changeset found

Latest commit: 43403f6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR 
Some errors occurred when validating the changesets config:
The package or glob expression "rrdom" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrdom-nodejs" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb-player" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrweb-snapshot" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@rrweb/types" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "@rrweb/web-extension" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
The package or glob expression "rrvideo" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

greptile-apps[bot]
greptile-apps bot reviewed May 17, 2024
View reviewed changes
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Summary

  • Dynamic Chart Title Update: The default title of the chart now dynamically updates to reflect the selected graphed value using a useRef to store a temporary metric view title.
  • Function Type and Metric Integration: The title is updated based on the selected function type and metric, ensuring it accurately represents the current data being graphed.

Notes

  • Code Reuse Opportunity: The useRef pattern for dynamic title updates could potentially be reused in other components requiring similar dynamic title functionality.

Comments

frontend/src/pages/Graphing/GraphingEditor.tsx

  • Line 137: Using useMemo to update tempMetricViewTitle.current is unconventional. Consider using a useEffect hook instead to ensure side effects are handled correctly.

@algora-pbc algora-pbc bot mentioned this pull request May 17, 2024
Closed
@rajeshj11
Copy link
Contributor Author

@Vadman97 Please take a moment to review it and let me know if any areas need improvement or adjustments. Your insights and suggestions would be greatly appreciated.

Vadman97
Vadman97 approved these changes May 17, 2024
View reviewed changes
Copy link
Member

@Vadman97 Vadman97 left a comment

Choose a reason for hiding this comment

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

thank you for your contribution! looks great, just adding a small tweak to the requirements to add the Source to the default string

frontend/src/pages/Graphing/GraphingEditor.tsx Outdated Show resolved Hide resolved
@Vadman97
Copy link
Member

@rajeshj11 could you please also run yarn format:all to lint the file? https://github.com/highlight/highlight/actions/runs/9123681747/job/25112098771?pr=8602

@rajeshj11
Copy link
Contributor Author

Screenshot from 2024-05-18 00-08-26

@rajeshj11
Copy link
Contributor Author

@rajeshj11 could you please also run yarn format:all to lint the file? https://github.com/highlight/highlight/actions/runs/9123681747/job/25112098771?pr=8602

Done

@rajeshj11 rajeshj11 requested a review from Vadman97 May 18, 2024 00:25
@Vadman97 Vadman97 enabled auto-merge (squash) May 18, 2024 00:26
@rajeshj11
Copy link
Contributor Author

@Vadman97 Are there any other changes required from my side?

@Vadman97
Copy link
Member

@Vadman97 Are there any other changes required from my side?

doesn't look like it - i think we need to fix one of the checks failing in remote form PRs. will fix it tomorrow

@Vadman97 Vadman97 merged commit ffe50d2 into highlight:main May 20, 2024
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greptile-apps greptile-apps[bot] greptile-apps left review comments

@Vadman97 Vadman97 Awaiting requested review from Vadman97

Assignees
No one assigned
Labels
🙋 Bounty claim
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dashboard should show default title based on metric being graphed
2 participants
@rajeshj11 @Vadman97