-
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
Plugins: Add grafana/user/profile/tab plugin extension point #77863
Plugins: Add grafana/user/profile/tab plugin extension point #77863
Conversation
plugin extension point
See grafana/plugin-tools#557 for docs update PR |
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.
generally looks good 👍
think we need to refactor slightly to make clear the separation between the tab query param (or id
) and the tab title (or title
). that's especially important for the core
tab, because we'll want to translate that so we can't rely on the title.
shout if anything doesn't make sense 👍
}); | ||
|
||
it.each([tabOneName, tabOneName.toUpperCase(), tabOneName.toLowerCase()])( | ||
'should set the active tab based on the "tab" query param and be case-insensitive', |
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.
why should it be case-insensitive? 🤔 afaik all our urls are supposed to be lower case
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 I was overthinking edge-cases here. Your proposed approach of distinguishing between tab ids (always lowercased) & titles makes things simpler imo 👍
</TabsBar> | ||
<TabContent> | ||
{activeTab === CORE_SETTINGS_TAB.toLowerCase() && <UserProfile />} | ||
{toPairs(groupedExtensionComponents).map(([title, pluginExtensionComponents]) => { |
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.
why use toPairs
instead of Object.entries
? 🤔
with the new structure, this will return [id, pluginExtensionComponents
instead of [title, pluginExtensionComponents]
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.
updated this to use Object.entries
, thanks for the suggestion! (this is how you know I'm (mostly) a backend engineer 🤓)
this:
Object.entries(groupedExtensionComponents).map(([title, pluginExtensionComponents])
will still return [title, pluginExtensionComponents]
since I'm grouping the registered extension point components here by title
. Nevertheless, I've added this small util function to convert a tab title to a tab id.
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.
accidentally approved when i meant to just leave a comment... so now i've gotta request changes 😂
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've pinged in our channel to see if we can get a second pair of 👀 on it just for a sanity check
one more thing @joshhunt pointed out, i think in https://raintank-corp.slack.com/archives/C064R17Q1A8/p1699964541932529 we discussed calling the default tab General
instead of Core
- can we make that change (and for the internal id as well). (sorry for not raising that sooner 🤦 )
@ashharrison90 no worries, I've updated "Core" -> "General". Question regarding the translation
I don't seem to have access to the project in crowdin (despite having access to grafana.crowdin.com). Anything more I need to do here or this is good as-is? |
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.
lgtm 🚀
let's see if we can get 1 more from @grafana/grafana-frontend-platform to approve as well 🙏
re: translations - nope, you're all set. all you need to make sure is that the english string (and pseudo string) is present, the empty strings in other languages is expected.
what happens is those keys are added to crowdin, then external translators will add the other language translations to crowdin. a github action then syncs the changes from crowdin back to the main repo.
const [queryParams, updateQueryParams] = useQueryParams(); | ||
const tabQueryParam = queryParams[TAB_QUERY_PARAM]; | ||
const [activeTab, setActiveTab] = useState<string>( | ||
typeof tabQueryParam === 'string' ? tabQueryParam : GENERAL_SETTINGS_TAB | ||
); |
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.
Why use a query parameter here and not separate routes?
# What this PR does Dependent on grafana/grafana#77863 ## Which issue(s) this PR fixes ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --------- Co-authored-by: Rares Mardare <[email protected]>
# What this PR does Dependent on grafana/grafana#77863 ## Which issue(s) this PR fixes ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --------- Co-authored-by: Rares Mardare <[email protected]>
What is this feature?
This PR adds a new plugin component extension point,
grafana/user/profile/tab
, allowing plugins to render content on the/profile
.Core
is the default tab and represents the pre-existing settings. The remaining tab(s) are dependant based on what plugin(s) have registered against this extension point. For example, the following code within a plugin would result in this:/profile
page will now look for atab
query parameter. If this is equal to any of the tab titles (case-insensitive), that tab will be rendered directly on page load.Note: multiple plugins can render content to the same tab. Tabs + tab content are decided based on unique
title
attributes. Any duplicates are grouped together and rendered within the same tab (this will be useful for example in the "IRM" use-case where Grafana OnCall and Grafana Incident may each need to render some shared IRM-level settings; example mobile app QR code).Why do we need this feature?
Grafana OnCall and Grafana Incident teams have a quarterly milestone to add Grafana Incident features to the IRM mobile app (previously OnCall mobile app). Part of this project involves moving the QR authentication code from within Grafana OnCall to somewhere "higher-up" within core Grafana. Since this is a shared setting between OnCall and Incident it made sense to move it to
/profile
.Who is this feature for?
Plugin developers.
Which issue(s) does this PR fix?:
This unblocks grafana/oncall#3296
Please check that: