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

added info link for Plugin Health Score #1331

Merged
merged 3 commits into from
Apr 15, 2023

Conversation

Harsh3341
Copy link
Contributor

#1320 (comment)

Info links for plugins is been added

@Harsh3341
Copy link
Contributor Author

Solved #1328

@NotMyFault
Copy link
Member

Please don't mark comments as resolved when they are not.

I recommend moving this to a dedicated element because clicking on the question mark links through the plugin page, rather than directly to the health score page, causing unneeded browser noise.

is still to address.

I'd recommend keeping the question mark clickable, additionally to the hover.

@Harsh3341
Copy link
Contributor Author

Harsh3341 commented Apr 13, 2023

I'd recommend keeping the question mark clickable, additionally to the hover.

It is clickable and also can be hovered

@NotMyFault
Copy link
Member

NotMyFault commented Apr 13, 2023

I'd recommend keeping the question mark clickable, additionally to the hover.

It is clickable

Why do you think so? You removed the logic for that, after my previous comment, for no reason.

@Harsh3341
Copy link
Contributor Author

How did you test this? On the preview site clicking the question mark does nothing.
IMG_20230413_121128

Currently I'm in my phone while clicking on "?" Tooltip is opening, also I have tested this on desktop while clicking and hover

@NotMyFault
Copy link
Member

NotMyFault commented Apr 13, 2023

I didn't mention the tooltip, I mentioned the linking action you previously had on the question mark. The need to hover first and click later just requires more action of the user, when there's no need for it.

@Harsh3341
Copy link
Contributor Author

I didn't mention the tooltip, I mentioned the linking action you previously had on the question mark. The need to hover first and click later just requires more action of the user, when there's no need for it.

O.. I misunderstood what you said before, but now I get it.

What I have done is I have removed link from "?" and added stopPropagation() when clicked on "?" So that it does not redirect to plugin page and also I have removed link because for phone users I want toolkit to open onclick rather then directly redirecting to Health Score page.

What needs to be done is I have to add stopPropagation() in tooltip health score

@NotMyFault
Copy link
Member

What I have done is I have removed link from "?" and added stopPropagation() when clicked on "?" So that it does not redirect to plugin page and also I have removed link because for phone users I want toolkit to open onclick rather then directly redirecting to Health Score page.

Yeah, I questioned the "why". This design causes more noise for the end user, especially if the hover is not sticky, if you're not fast quick, you don't make it:

Kapture.2023-04-13.at.09.10.31.mp4

@Harsh3341
Copy link
Contributor Author

Noted...

With all fixes, I will push the next commit

@Harsh3341
Copy link
Contributor Author

Harsh3341 commented Apr 13, 2023

Yeah, I questioned the "why". This design causes more noise for the end user, especially if the hover is not sticky, if you're not fast quick, you don't make it:

just now checked the code its reactstrap tooltip component causing that issue of hover, tested some changes but still result remains the same, so should I remove the tooltip component and rewrite a different code for it?

@NotMyFault
Copy link
Member

Yeah, I questioned the "why". This design causes more noise for the end user, especially if the hover is not sticky, if you're not fast quick, you don't make it:

just now checked the code its reactstrap tooltip component causing that issue of hover, tested some changes but still result remains the same, so should I remove the tooltip component and rewrite a different code for it?

None of that is needed, and out of the scope of the PR. All I asked for is re-adding the ability to click the question mark.

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -24,6 +57,7 @@ PluginHealthScore.propTypes = {
healthScore: PropTypes.shape({
value: PropTypes.number,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
value: PropTypes.number,
value: PropTypes.number.isRequired,

@@ -24,6 +57,7 @@ PluginHealthScore.propTypes = {
healthScore: PropTypes.shape({
value: PropTypes.number,
}),
name: PropTypes.string,
Copy link
Member

@halkeye halkeye Apr 15, 2023

Choose a reason for hiding this comment

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

you don't handle name not being provided, so it should be required

Suggested change
name: PropTypes.string,
name: PropTypes.string.isRequired,

Copy link
Member

Choose a reason for hiding this comment

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

@Harsh3341 do you want to do a followup pr with these? I'm just going to merge.

@halkeye halkeye merged commit 8fb11c7 into jenkins-infra:master Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants