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

Move alerts to top and update their styles #386

Merged
merged 1 commit into from
Oct 17, 2020

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Oct 9, 2020

This PR tries to address some issues with the alerts:

  • Current security warnings more hidden than previous version warnings – fixed by showing security warnings in full
  • Minor design issues with the alert styles – fixed by using bootstrap standard classes
  • Warnings related to invalid table markup – fixed by not using tables for layout

image

Multiple vulnerabilities

image

Deprecation notice

image

CC @daniel-beck

@zbynek zbynek requested a review from a team as a code owner October 9, 2020 19:26
@zbynek zbynek marked this pull request as draft October 9, 2020 19:27
@halkeye
Copy link
Member

halkeye commented Oct 9, 2020

Minor design issues with the alert styles -- fixed by using bootstrap standard classes

<3 so much <3 using the standard items

Warnings related to invalid table markup -- fixed by not using tables for layout

same

<div className="alert alert-warning alert-with-icon">
<span className="alert-icon warning" aria-label="Warning"/>
<b>Deprecated:</b>
{'This plugin has been marked as deprecated. '}
Copy link
Member

Choose a reason for hiding this comment

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

FYI, you can use a backtick and make this one large blob, like java's 3 quotes. At some point I want to find out why eslint messes with strings

(I know you didn't make these strings just reformatted, but wanted to share)

}
#pluginPage #grid-box .badge-box .badge.warning {
.security-warning-list {
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason this isn't scoped? is it used in multiple places?

@daniel-beck
Copy link
Contributor

daniel-beck commented Oct 12, 2020

I like this a lot. Thanks!

Could you check to make sure this looks reasonable even when a plugin has multiple active security warnings?

Two warnings can happen fairly easily and there are several plugins that have that, like https://plugins.jenkins.io/perfecto/ or https://plugins.jenkins.io/elastest/

More than that is less relevant (so doesn't need to look all that reasonable), but just for giggles could you take a screenshot of https://plugins.jenkins.io/websphere-deployer/ ?


I wonder whether a different phrasing for the affected versions would make sense.

Currently, if a warning is defined for an issue that has not been resolved, we still declare what the latest version is (future releases are undetermined). That could be misleading to users — any active security warning should be considered to affect the current release, even if this metadata doesn't like it.

For example, the site could detect whether the regex matches anything, and if so, word the message differently.

Perhaps not immediately related to this PR, but given the additional prominence of the version information, especially being shown at the same time as the latest version on the page, this seems different from before.

@halkeye
Copy link
Member

halkeye commented Oct 12, 2020

image

one of the many reasons I'd love to see this hooked up to netlify is that it would build demo sites per PR. I wonder how hard it would be to do that with azure sites.

@timja
Copy link
Member

timja commented Oct 12, 2020

This is Azure's service for it I believe: https://azure.microsoft.com/en-gb/services/app-service/static

@zbynek zbynek marked this pull request as ready for review October 14, 2020 04:13
@zbynek
Copy link
Contributor Author

zbynek commented Oct 14, 2020

@daniel-beck I suggest to remove the intervals altogether: currently they either say "afffects some versions" or "Affects version X and earlier" where X is the current version number. There is no actual difference between those two statements, it's just a bit confusing. Please see the updated screenshots in the initial comment and let me know if the wording is OK.

@halkeye
Copy link
Member

halkeye commented Oct 17, 2020

@zbynek is this still waiting on something? I say merge and we can always follow up with more changes, it looks really good as is.

@daniel-beck
Copy link
Contributor

@zbynek It looks good but may be slightly misleading if newer releases have been published since (for which it's essentially undefined, sometimes it's fixed, sometimes not). Overall I think this is a reasonable solution.

FWIW "some versions" has basically been retired as we now always specify the most recent known affected version so that never occurs anymore.

@timja
Copy link
Member

timja commented Oct 17, 2020

@zbynek It looks good but may be slightly misleading if newer releases have been published since (for which it's essentially undefined, sometimes it's fixed, sometimes not). Overall I think this is a reasonable solution.

update center data should be updated then to remove the warning if fixed ^.^.
Users shouldn't have to figure out if it's fixed or not ideally

@zbynek
Copy link
Contributor Author

zbynek commented Oct 17, 2020

Overall I think this is a reasonable solution.

@daniel-beck OK to merge as is?

@halkeye halkeye merged commit 5351448 into jenkins-infra:master Oct 17, 2020
@zbynek zbynek deleted the alerts branch April 12, 2022 23:34
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