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

feat(integration-directory): plugin detailed view #16821

Merged
merged 18 commits into from Feb 5, 2020

Conversation

scefali
Copy link
Member

@scefali scefali commented Feb 5, 2020

This PR adds the detailed view for plugins in the new integration directory. Here are some nifty screenshots:

Information tab:

Screen Shot 2020-02-05 at 10 55 41 AM

Project Config tab:

Screen Shot 2020-02-05 at 11 02 55 AM

You'll notice that the information tab contains information about the features that was added in this PR #16797. We pull them off the feature_descriptions of the plugin classes.

For the configurations tab, I decided to call it Project Configurations so it's a bit more clear that these are project specific. The items you see in the rows have Project Badges so they look like projects elsewhere in Sentry.

You'll notice we have the following buttons in the rows:

  • Enable: enables a plugin that has already been configured
  • Configure: takes you to the configuration page of that plugin for that project
  • Uninstall: resets the configuration and disables the plugin for that project/plugin

At the top we have the Add to Project button. Clicking it generates the project select modal:

Screen Shot 2020-02-05 at 11 03 01 AM

Doesn't look that great now but we will probably improve it once we have a proper design.

@scefali scefali requested review from NisanthanNanthakumar and a team February 5, 2020 19:12
@@ -105,6 +105,7 @@ def get(self, request, organization):
"projectName": project.name, # TODO(steve): do we need?
"enabled": plugin_info["enabled"],
"configured": plugin_info["configured"], # TODO(steve): do we need?
"projectPlatform": project.platform,
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed to add this so we can render the project badge

</Button>
) : (
<StyledButton
borderless
Copy link
Member

Choose a reason for hiding this comment

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

Why borderless in this context?

Copy link
Member Author

Choose a reason for hiding this comment

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

@evanpurkhiser It definitely doesn't look good here, that's for sure. We really just copy-pasted these buttons from the existing integrations page where it looks better:

Screen Shot 2020-02-05 at 11 20 21 AM

Right now, we are waiting for a proper design from Chris. We'll tune up all the styling on this page once we know what it should look like.

Comment on lines 146 to 153
const features = plugin.featureDescriptions.map(f => ({
featureGate: f.featureGate,
description: (
<FeatureListItem
dangerouslySetInnerHTML={{__html: singleLineRenderer(f.description)}}
/>
),
}));
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to use dangerouslySetInnerHTML because the descriptions have actual HTML tags in them? If so, isn't it risky to allow users to write HTML that would show up on Sentry?

Copy link
Member

Choose a reason for hiding this comment

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

It's markdown, the markdown parser prevents HTML

Copy link
Member Author

Choose a reason for hiding this comment

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

@leeandher Good point, I actually don't think in this case we need to use that here. But just so you know, singleLineRenderer will actually sanitize the inputs so it wouldn't be risky.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh okay, thanks guys

Comment on lines +140 to +146
getTabDiplay(tab: Tab) {
//we want to show project configurations to make it more clear
if (tab === 'configurations') {
return 'project configurations';
}
return tab;
}
Copy link
Member

Choose a reason for hiding this comment

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

This function, or where it's implemented should be surrounded by the t() function, right?

Copy link
Member

Choose a reason for hiding this comment

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

The raw strings are always the things that should be wrapped in t(), using a variable in t defeats it, since we can't map raw strings to translations then

Copy link
Member Author

Choose a reason for hiding this comment

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

@leeandher Good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

@evanpurkhiser Is that true? What if we have translations for the different values of the input of t()?

@@ -40,7 +40,7 @@ export default class PluginRow extends React.Component<Props> {
<ProviderDetails>
<Status enabled={this.isEnabled} />
<StyledLink
to={`/settings/${slug}/plugins/${plugin.slug}?tab=configurations`}
to={`/settings/${slug}/plugins/${plugin.slug}/?tab=configurations`}
Copy link
Member Author

Choose a reason for hiding this comment

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

had to add the trailing / so the link works properly

@scefali scefali merged commit 1e53431 into master Feb 5, 2020
@scefali scefali deleted the feat/plugin-detailed-view branch February 5, 2020 23:13
@benvinegar
Copy link
Contributor

Dang, looks good!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants