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

Add tooltip for non-compliant saptune #2319

Merged
merged 5 commits into from
Feb 16, 2024
Merged

Conversation

rtorrero
Copy link
Contributor

@rtorrero rtorrero commented Feb 15, 2024

Description

This PR adds a tooltip to add further details when saptune is not compliant

Preview:
Captura desde 2024-02-15 15-37-12

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Just a comment.
If @jagabomb agrees that the tooltip needs to go in the bottom is ok from my side

@@ -9,7 +10,12 @@ function SaptuneTuningState({ state }) {
case 'not compliant':
return (
<div className="flex">
<HealthIcon health="critical" />
<Tooltip
content="Run `saptune note verify` in the host for further details"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think markdown works here as it is. You would need to wrap it up, something like:

<ReactMarkdown className="markdown" remarkPlugins={[remarkGfm]}>
Run `saptune note verify` in the host for further details
 </ReactMarkdown>

@jagabomb
Copy link
Contributor

Just a comment. If @jagabomb agrees that the tooltip needs to go in the bottom is ok from my side

Something like this would be good...
Screenshot 2024-02-15 at 17 16 35

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

If @jagabomb is happy, I'm happy.
Jurgen, you could check it by yourself in saptune details view storybook, but you would need to change the tuning value to not compliant in the options

@rtorrero
Copy link
Contributor Author

As discussed with @arbulu89 we ended up removing the markdown as it adds very little value and looks quite odd

Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

LGTM!

@jagabomb
Copy link
Contributor

@rtorrero is it possible to wrap the text over two lines like screenshot below instead of one long line?
Screenshot 2024-02-15 at 18 14 02

@rtorrero
Copy link
Contributor Author

@jagabomb should be good now:
Captura desde 2024-02-16 08-54-12

Copy link
Contributor

@jagabomb jagabomb left a comment

Choose a reason for hiding this comment

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

LGTM!

@rtorrero rtorrero merged commit 9335e6b into main Feb 16, 2024
24 checks passed
@rtorrero rtorrero deleted the noncompliant-saptune-tooltip branch February 16, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants