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

Moved the plugin name above all the tabs and made it common for all tabs #363

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

sachinmukherjee
Copy link
Contributor

Related to issue #306

Summary of this pull request: Moved the plugin name above all tabs and made it common for all tabs and also made version in a separate row.
image

@oleg-nenashev Please review this PR

@sachinmukherjee sachinmukherjee requested a review from a team as a code owner September 19, 2020 16:19
@@ -60,10 +63,12 @@ function PluginPage({data: {jenkinsPlugin: plugin}}) {
</ul>
<div className="padded">
{state.selectedTab === 'documentation' && (<>
<h1 className="title">
{cleanTitle(plugin.title)}
<h1 className="title-details">
Copy link
Member

Choose a reason for hiding this comment

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

There should only be one h1 on the page, this section doesn’t appear to need to be a heading at all.

Can you add a screenshot of a plugin with active security warnings too?

Copy link
Contributor

@zbynek zbynek Sep 19, 2020

Choose a reason for hiding this comment

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

Changing h1 into something more suitable would also help remove all this clutter from the heading in Google search results
https://www.google.com/search?q=jenkins+git+plugin&oq=jenkins+git+plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @timja, I tried to find some plugin with a security warning but didn't succeed. Can you help me out with some plugin names that have some security warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zbynek I tried the custom job icon plugin but it is not showing any security warning in my development env.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After following the above steps security warning is still not coming up

Copy link
Contributor

@zbynek zbynek Sep 22, 2020

Choose a reason for hiding this comment

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

@timja the screenshot of a plugin with a warning is above -- do you think it's OK for the scope of this PR or should something be done here? Personally I think it's weird that current warnings are hidden (you need to click the triangle) and warnings for old versions are shown directly. It might make sense to kill the triangle entirely and show the current warning as an alert similar to the adoption message. Maybe out of scope here?

Copy link
Member

Choose a reason for hiding this comment

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

the above looks ok I think, (in comparison to the previous)

yes it's very weird and needs a re-design

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I commit my files with the above mentioned changes?

Copy link
Member

Choose a reason for hiding this comment

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

yes please

Copy link
Contributor

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @sachinmukherjee !

@timja timja changed the title moved the plugin name above all the tabs and made it common for all tabs Moved the plugin name above all the tabs and made it common for all tabs Sep 23, 2020
@timja timja merged commit d81b16b into jenkins-infra:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants