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

Alerting: Add templates autocomplete #53655

Merged
merged 21 commits into from Sep 20, 2022

Conversation

konrad147
Copy link
Contributor

@konrad147 konrad147 commented Aug 12, 2022

What this PR does / why we need it:
This PR adds a basic autocomplete feature to the template's editor.
It also includes inline documentation for available template data

Which issue(s) this PR fixes:
Fixes #35726

Special notes for your reviewer:
Kapture 2022-09-15 at 13 00 11

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@konrad147 konrad147 self-assigned this Sep 5, 2022
@konrad147 konrad147 added add to changelog no-backport Skip backport of PR labels Sep 5, 2022
@konrad147 konrad147 added this to the 9.2.0 milestone Sep 5, 2022
@konrad147 konrad147 marked this pull request as ready for review September 5, 2022 11:26
@konrad147 konrad147 requested a review from a team as a code owner September 5, 2022 11:26
@konrad147
Copy link
Contributor Author

@jessover9000 Please take a look at the Templates guideline section and the table to the right (it contains a list of all available template data)

@brendamuir Please take a look at the user-facing messages. Can we improve them?

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

…rting/35726-template-editor-autocomplete' of github.com:grafana/grafana into alerting/35726-template-editor-autocomplete
@grafanabot
Copy link
Contributor

Copy link
Contributor

@brendamuir brendamuir left a comment

Choose a reason for hiding this comment

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

Looks fine to me now.

@maskin25
Copy link
Member

maskin25 commented Sep 8, 2022

LGTM!

@jessover9000
Copy link
Contributor

jessover9000 commented Sep 12, 2022

Hello, sorry for the late comment, I somehow missed the Github notification. So from a UX perspective, I personally found it a bit difficult to make sense of everything that’s going on here. I have only seen the screenshot and not the whole thing, so I still have a lot of questions and don't fully understand everything. Maybe I need to see a demo and have a call about this 😅

  1. The templating guideline
    What exactly are the blue words in the top? Are they those "snippets" or is the content of the template code box what we refer to as "snippets"? Are the blue words links? Why are they blue, since the other links are grey and underlined? What happens if I interact with them / can I interact with them?

  2. The table
    I was struggling to match stuff from the table on the right to the code/understand how the table relates to the code. What is the purpose of the table? If you're a user with little or no Go templating experience, how would you use this view and the table to accomplish writing/modifying your first template? As a user I might prefer getting contextual help about each string/value/etc right in the code by hovering or clicking or something, because right now it's kind of a lot of work to find either the data from the table in the code or the stuff from the code in the table. I feel like as a user I might actually profit more from seeing both the code and a notification preview side by side to understand what e.g. "Receiver" would be spelled out to in my actual notification. Might also be faster than searching for properties in that table. If we stick to the table, the least we could do is sort the values alphabetically.
    Also it looks like there is overlap between general template data and alert template data, e.g. with "status" being there in both cases and basically behaving the same? I haven't seen the whole table but maybe it can be optimized.

@gillesdemey
Copy link
Member

I think we'll need to work on the table view a bit as it looks a bit funky on smaller resolutions
image

const insertFallback = typeof label === 'string' ? label : label.label;
const labelObject = typeof label === 'string' ? { label: label, description: detail } : { ...label };

labelObject.description ??= detail;
Copy link
Member

Choose a reason for hiding this comment

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

I've never really seen a good use-case for logical nullish assignments, why do we want to assign description only if it's null here?

Copy link
Contributor Author

@konrad147 konrad147 Sep 13, 2022

Choose a reason for hiding this comment

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

I want to use detail only as a fallback value if description doesn't exist. For some suggestions I wanted the description to be different from the detail. Simply skipping the description for some entries causes a worse user experience.
Suggestions look better in the editor if configured this way

@grafanabot
Copy link
Contributor

@konrad147
Copy link
Contributor Author

Thanks for your feedback @jessover9000!

  1. I improved the guideline. They are snippets, users are able to use them inside the editor. I changed their look so they don't resemble links (they are not interactive)

If you're a user with little or no Go templating experience, how would you use this view and the table to accomplish writing/modifying your first template?

If the user has experience with any templating language, they will be looking for this table. It references what data is available for use inside the template.
If the user doesn't have any experience in templating at all they should go and read the documentation first (the templating guideline directs there. The docs contain links to more detailed resources about templating)

As a user I might prefer getting contextual help about each string/value/etc right in the code by hovering or clicking or something,

The autocomplete feature does it to some extent.

I feel like as a user I might actually profit more from seeing both the code and a notification preview side by side to understand what e.g. "Receiver" would be spelled out to in my actual notification.

Definitely! We've done already research in this regard yet it's a pretty complex task, beyond the scope of this one

If we stick to the table, the least we could do is sort the values alphabetically.

We sort them in the same way in our docs and I feel the order is from the most important (most frequently used) to the least important ones

@grafanabot
Copy link
Contributor

Copy link
Member

@gillesdemey gillesdemey left a comment

Choose a reason for hiding this comment

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

This looks amazing, LGTM! 🚀

@konrad147 konrad147 merged commit 4c4b758 into main Sep 20, 2022
@konrad147 konrad147 deleted the alerting/35726-template-editor-autocomplete branch September 20, 2022 11:07
@dsotirakis dsotirakis modified the milestones: 9.2.0, 9.2.0-beta1 Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Alerting: Add autocomplete to template editor to visualize available variables
8 participants