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
Prometheus: Fix link to Prometheus graph in dashboard #29543
Conversation
@@ -18,6 +18,14 @@ interface State { | |||
export default class PromLink extends Component<Props, State> { |
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.
Would it be too much to ask to refactor this component to a function component? 😄
2 things I noticed
- the same logic is used in componentDidMount and componentDidUpdate
- getExternalLink is async because?
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.
No, not much work at all. And you are totally right ⭐️ - leave the code in better shape than.... So I refactored it to hooks + as I was adding tests, I switched to react-testing-library. 🙂
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.
Also async was not necessary.
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.
Thank you! This looks awesome with the refactor. Good job. 🐥
* Fix Promlink to have href after componentDidMount * Refactor to hooks and react testing library * Update * Update snapshot test
What this PR does / why we need it:
The href was set only in componentDidUpdate. Therefore to have href correct, you've had to interact with editor. This PR fixes it + adds test coverage.
Which issue(s) this PR fixes:
Fixes #29444