Skip to content

Replace data alert plugin#3529

Merged
Simrayz merged 4 commits intomasterfrom
chore/3481-replace-data-alert-plugin
Sep 12, 2025
Merged

Replace data alert plugin#3529
Simrayz merged 4 commits intomasterfrom
chore/3481-replace-data-alert-plugin

Conversation

@Simrayz
Copy link
Copy Markdown
Contributor

@Simrayz Simrayz commented Sep 10, 2025 •
edited by johannaengland
Loading

Scope and purpose

Resolves #3481.

This PR replaces the functionality in the data-alert plugin in Foundation.

I looked at the Foundation 5 foundation.alert.js source code, and all the plugin does it animate the removal of the alert when an element with .close is clicked. As such, I added a new alert.js plugin which does exactly this.

To explicitly remove the effects of foundation's [data-alert], I opted to use a new attribute: [data-nav-alert]. Once we remove all foundation js, we can consider renaming the plugin [data-alert] again.

Considerations

The [data-alert] plugin was used on elements that don't have close buttons, such as "Metric list" or "Port sensors" (see image) on this port on an ip device

image

I'm not sure if this was intentional or if they are supposed to have close buttons, so I just left it as-is for the time being.

This pull request

  • Adds new [data-nav-alert] plugin to replace Foundation 5 Alert JS
  • Replace all usages og [data-alert] with [data-nav-alert]
  • Fix bug in network explorer where alerts are not shown after ajax requests
  • Added transition styles for [data-nav-alert] in _override.scss, and removed underline from close icons.

Reproduce the Networkexplorer bug

  • To reproduce the bug in the Networkexplorer view, just try to perform an invalid search.
  • Enter an invalid IP address in the search, select IP in the dropdown, and Exact search.
  • An alert with "Search failed!" should be visible, and the close button removes the alert with a transition.
image

Contributor Checklist

Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to NAV can be found in the
Hacker's guide to NAV.

  • Added a changelog fragment for towncrier
  • Added/amended tests for new/changed code
  • Added/changed documentation
  • Linted/formatted the code with ruff, easiest by using pre-commit
  • The first line of the commit message continues the sentence "If applied, this commit will ...", starts with a capital letter, does not end with punctuation and is 50 characters or less long. See https://cbea.ms/git-commit/
  • This pull request is based on the correct upstream branch: For a patch/bugfix affecting the latest stable version, it should be based on that version's branch (<major>.<minor>.x). For a new feature or other additions, it should be based on master.
  • If applicable: Created new issues if this PR does not fix the issue completely/there is further work to be done
  • If this results in changes in the UI: Added screenshots of the before and after
  • If this adds a new Python source code file: Added the boilerplate header to that file

@Simrayz Simrayz self-assigned this Sep 10, 2025
@Simrayz Simrayz requested a review from a team September 10, 2025 11:53
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 10, 2025 •
edited
Loading

Test results

   12 files     12 suites   12m 37s ⏱️
2 511 tests 2 511 ✅ 0 💤 0 ❌
7 068 runs  7 068 ✅ 0 💤 0 ❌

Results for commit 3cd730f.

♻️ This comment has been updated with latest results.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 10, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.02%. Comparing base (5a7477e) to head (3cd730f).
⚠️ Report is 295 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #3529   +/-   ##
=======================================
  Coverage   62.02%   62.02%           
=======================================
  Files         611      611           
  Lines       44850    44850           
  Branches       43       43           
=======================================
  Hits        27818    27818           
  Misses      17022    17022           
  Partials       10       10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

johannaengland
johannaengland requested changes Sep 11, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@johannaengland johannaengland left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very well documented PR!
I agree that we should rename it back to [data-alert] when foundation is removed, please make a follow up issue

And I realized that your bugfix is actually just masking an underlying bug that happens when an invalid IP address is entered for the exact IP search. That bug is happening in the backend, I am working on a fix, afterwards we can check if that bugfix in the frontend is still necessary

@johannaengland johannaengland mentioned this pull request Sep 11, 2025
Merged
6 tasks
@johannaengland
Copy link
Copy Markdown
Contributor

johannaengland commented Sep 11, 2025

And I realized that your bugfix is actually just masking an underlying bug that happens when an invalid IP address is entered for the exact IP search. That bug is happening in the backend, I am working on a fix, afterwards we can check if that bugfix in the frontend is still necessary

I have added #3534. Please have a look at how this fix would influence this PR if we decide to merge that.

@Simrayz
Copy link
Copy Markdown
Contributor Author

Simrayz commented Sep 11, 2025

And I realized that your bugfix is actually just masking an underlying bug that happens when an invalid IP address is entered for the exact IP search. That bug is happening in the backend, I am working on a fix, afterwards we can check if that bugfix in the frontend is still necessary

I have added #3534. Please have a look at how this fix would influence this PR if we decide to merge that.

The "fix" in this PR is to remove the if block, as it requires a re-render (with errors in the view) to display errors. Instead it should just show an empty div as a target that the ajax-handler can add an alert to. Your fix in #3534 (with proper http status code added) will enable us to show a more specific error message, but otherwise won't conflict.

johannaengland
johannaengland approved these changes Sep 12, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Happy with this now! Please make a follow up issue for after #3534 is merged to show that error message

@Simrayz Simrayz force-pushed the chore/3481-replace-data-alert-plugin branch from 76cbd7e to 3cd730f Compare September 12, 2025 07:47
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Sep 12, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Simrayz Simrayz merged commit d362866 into master Sep 12, 2025
17 checks passed
@Simrayz Simrayz deleted the chore/3481-replace-data-alert-plugin branch September 12, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johannaengland johannaengland johannaengland approved these changes

Assignees

@Simrayz Simrayz

Labels

cleanup javascript

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replace Foundation Alerts with custom implementation

2 participants

@Simrayz @johannaengland