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

Fix navigation still possible from host details to SUSE Manager pages in case of error #3061

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

dottorblaster
Copy link
Contributor

Description

Fixing navigation still possible from host details to suma pages in case of error.

How was this tested?

Existing tests passing, I would like to add a Jest test to ensure it doesn't go off anymore.

@dottorblaster dottorblaster added the bug Something isn't working label Oct 10, 2024
@dottorblaster dottorblaster force-pushed the suse-manager-indicator-navigation-fix branch from dabe3a1 to 97735ba Compare October 18, 2024 08:37
@dottorblaster dottorblaster marked this pull request as ready for review October 21, 2024 09:08
@dottorblaster dottorblaster force-pushed the suse-manager-indicator-navigation-fix branch from 32276eb to 7c73af1 Compare October 21, 2024 09:10
@dottorblaster dottorblaster force-pushed the suse-manager-indicator-navigation-fix branch from 7c73af1 to 2af3b0b Compare October 21, 2024 10:02
Copy link
Member

@janvhs janvhs left a comment

Choose a reason for hiding this comment

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

Your changes are perfectly fine. I am just concerned in regards to the role attribute, because I don't think "button" is correct when doing navigation. Just gut feeling, didn't look it up yet

@@ -16,19 +16,21 @@ function Indicator({
isError,
onNavigate,
}) {
const clickHandler = isError ? () => {} : onNavigate;

return (
<Tooltip isEnabled={isError} content={tooltip} wrap={false}>
<div
role="button"
Copy link
Member

Choose a reason for hiding this comment

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

Just realised: should this change to a link when navigating?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to have clickable elements that trigger an event accessible also through keyboard a role="button" has to be specified, it was not my first course of action but eslint was pretty clear about that 😄

We can revisit it in the future if we see fit, thanks for the tip 👍

@dottorblaster dottorblaster merged commit 923a140 into main Oct 21, 2024
30 checks passed
@dottorblaster dottorblaster deleted the suse-manager-indicator-navigation-fix branch October 21, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants