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

chore: sonarqube fixes #3182

Merged
merged 8 commits into from
Jan 14, 2025
Merged

chore: sonarqube fixes #3182

merged 8 commits into from
Jan 14, 2025

Conversation

jenniferarnesen
Copy link
Collaborator

@jenniferarnesen jenniferarnesen commented Jan 14, 2025

SonarQube analysis reported issues on the latest code changes. Even though this is not a required check, we should comply when the suggestions are sensible. Here's a link to the findings: https://sonarcloud.io/summary/new_code?id=dhis2_dashboard-app&branch=master

Summary of changes and the quality that was flagged by Sonarqube:

  • ActionsBar: break up nested ternary (maintainability)
  • InformationBlock.module.css and SlideshowControlbar.module.css: missing generic font, but since this is declared higher up in the html structure for all elements (*), font-family declarations can be removed all-together in the app (reliability)
  • NavigationMenu.js: remove role="listitem" on li element since it is is implicit (reliability, maintainability)
  • ContentMenuItem.js: non-interactive element needs key handler. Fix was to make it semantically correct and switch to an interactive element button (reliability, maintainability)
  • StartScreen.module.css: remove shorthand style that followed a more specific style (reliability)
  • spec files - use object spread instead of Object.assign (conventional, maintainability)

What needs to be tested:

  • start screen: that the bullets on the list of recent dashboards looks the same
  • edit view item selector: that the launch link of each visualization looks the same and still works
  • that the slideshow button has the correct tooltip content
  • InformationBlock and Slideshow controller have the correct font
  • Dashboard dropdown "No dashboards found" looks correct

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Jan 14, 2025

🚀 Deployed on https://pr-3182.dashboard.netlify.dhis2.org

@dhis2-bot dhis2-bot temporarily deployed to netlify January 14, 2025 09:08 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify January 14, 2025 09:38 Inactive
Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

I left two suggestions in the code, but nothing critical.

I also compared the Netlify-deployed app with the app on https://test.e2e.dhis2.org/analytics-41dev/ and did spot a small regression.

edit view item selector: that the launch link of each visualization looks the same and still works

The functionality is fine, but the cursor style is now a bit odd (on MacOS at least). The launch-link has a default cursor while the menu item has a cursor: pointer;. On the test instance they both have cursor: pointer;. FYI: it'd be good to also show a background color change on hover as a visual aid for the user, but I guess that's out-of-scope for now...

One other note: I did not run the branch locally and could not work out how to test the following point on Netlify:

Dashboard dropdown "No dashboards found" looks correct

But since I could not actually spot any potentially relevant file changes in the PR diff, I just assumed this point would be fine.
I have tested this point too and it looked good.

@dhis2-bot dhis2-bot temporarily deployed to netlify January 14, 2025 12:22 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify January 14, 2025 12:35 Inactive
@jenniferarnesen
Copy link
Collaborator Author

I left two suggestions in the code, but nothing critical.

I also compared the Netlify-deployed app with the app on https://test.e2e.dhis2.org/analytics-41dev/ and did spot a small regression.

edit view item selector: that the launch link of each visualization looks the same and still works

The functionality is fine, but the cursor style is now a bit odd (on MacOS at least). The launch-link has a default cursor while the menu item has a cursor: pointer;. On the test instance they both have cursor: pointer;. FYI: it'd be good to also show a background color change on hover as a visual aid for the user, but I guess that's out-of-scope for now...

One other note: I did not run the branch locally and could not work out how to test the following point on Netlify:

Dashboard dropdown "No dashboards found" looks correct

But since I could not actually spot any potentially relevant file changes in the PR diff, I just assumed this point would be fine. I have tested this point too and it looked good.

Addressed. The button now uses cursor pointer on hover, and the color of the icon is darker on hover. Nice improvement if you ask me...

@dhis2-bot dhis2-bot temporarily deployed to netlify January 14, 2025 13:43 Inactive
@jenniferarnesen jenniferarnesen merged commit e8d563a into master Jan 14, 2025
29 checks passed
@jenniferarnesen jenniferarnesen deleted the chore/sonarcube-fixes branch January 14, 2025 14:05
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