-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Dashboard: Update query group options #63138
Conversation
@dprokop Hello, would you mind assisting with reviewing this change? |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
@JoaoSilvaGrafana @kaydelaney. Hey, Would you be able to take a look and provide some feedback? Thank you for the time. |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
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.
Thanks for your contibution @songhn233, and sorry for such a long wait time!
I've left some nits to fix, other than that the PR looks good!
This comment was marked as resolved.
This comment was marked as resolved.
@dprokop Thanks for your reply and code review. I have committed the changes according to your suggestions. I also added a comment. Could you please review to see if any further modifications are 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.
Hey @songhn233 👋🏾, thank you for your contribution. I've reviewed the PR and it looks good to me. However, there's a minor change that needs to be addressed. Also, please make sure to merge this branch with the latest main branch. :D
tooltip={ | ||
<> | ||
Overrides the relative time range for individual panels, which causes them to be different than what is | ||
selected in the dashboard time picker in the top-right corner of the dashboard. |
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.
🤔 I think we could add an example of values that we support. This will help the user to know what is possible and what kind of relative format they should put. Maybe, something like:
For example to configure the Last 5 minutes the Relative time should be
now-5m
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.
Hey, I made some minor changes including adding examples and updating the time shift
placeholder to match the other configurations.
Co-authored-by: Dominik Prokop <[email protected]>
Co-authored-by: Dominik Prokop <[email protected]>
7100516
to
500c5d3
Compare
Hey @axelavargas, Thanks for your review. I have added some additional explanations for |
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.
Hey @songhn233 👋🏾, thank you once again for implementing the updates. I dedicated some time to review the modifications you made concerning the placeholders and have left a few comments 😄.
Co-authored-by: Dominik Prokop <[email protected]>
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.
Hey @songhn233 , thanks for the updates on the changes – they look fantastic! 🌟 From my perspective, this PR is ready. We just need @dprokop to grant approval (since he requested changes earlier, and we can't merge without it 🙂).
* Dashboard: update query group options * chore: remove unused `console` Co-authored-by: Dominik Prokop <[email protected]> * fix: update placeholder Co-authored-by: Dominik Prokop <[email protected]> * feat(QueryGroupOptions): add tooltip example values * feat(QueryGroupOptions): update interval tooltip * Update public/app/features/query/components/QueryGroupOptions.tsx Co-authored-by: Dominik Prokop <[email protected]> * Update QueryGroupOptions.tsx --------- Co-authored-by: Dominik Prokop <[email protected]>
What is this feature?
Relative time
andTime shift
, the exact values may need to be discussed, my modifications are what I think is appropriate.Why do we need this feature?
Modified the placeholder
Max data points
andMin interval
(or all fields except for the time-related field) are real values, otherwise, they are a null text description (No limit
is displayed whenMin interval
is missing)So two fields about time originally set to
1h
may lead to the interpretation that they take exactly1h
(but are null) in this context, and such a placeholder setting may lead to misunderstandings, so it can be set to a null literal likeNo limit
.For
Time shift
set to0h
because it runs as0h
when it is null, and also implies the format of the input (math string)Before:
![image](https://user-images.githubusercontent.com/47357585/217627453-6c73d370-dac6-4e68-9d47-b62cce60264f.png)
After:tooltip and center styles
Other fields have corresponding descriptions, while the meaning of the time-related fields is not quite easy to understand, so some of the descriptions are quoted from the documentation.