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

Configurable Connection Notifications #1490

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

cgjgh
Copy link
Contributor

@cgjgh cgjgh commented Nov 20, 2024

Description

Adds configurability to connection notification.

Related Issue(s)

Resolves #811

Configuration option on the ui-base seems an easy middle ground here.
Default should be "on" in my opinion.

Originally posted by @joepavitt in #811 (comment)

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@cgjgh cgjgh force-pushed the UI-Base-Notification-Config branch 2 times, most recently from 7070f21 to b1b0f10 Compare November 20, 2024 15:22
@cgjgh cgjgh marked this pull request as draft November 20, 2024 15:29
@cgjgh cgjgh force-pushed the UI-Base-Notification-Config branch from b1b0f10 to 4bdadb4 Compare November 20, 2024 16:08
@cgjgh cgjgh marked this pull request as ready for review November 20, 2024 16:17
@joepavitt
Copy link
Collaborator

The use of localStorage is a bit tricky. The main.mjs is firing first, before App.vue where the LocalStorage is set, but this wouldn't have been noticed as the setting would have been saved/applied to the local storage in a previous refresh.

Instead, we should be utilising the vuex store here.

I'll make the relevant modifications that build on this great foundation

ui/src/App.vue Outdated Show resolved Hide resolved
@joepavitt
Copy link
Collaborator

@cgjgh thanks again for your work on this, checkout 80f3543 for the changes I've made in transferring this into our state store, rather than the local storage

@cgjgh
Copy link
Contributor Author

cgjgh commented Dec 3, 2024

@joepavitt Ok I see.. I initially thought that the Vuex store wasn't updated with the dashboard config at the time the socket events were set up, which is why I resorted to using localStorage for applying notification visibility settings.

this wouldn't have been noticed as the setting would have been saved/applied to the local storage in a previous refresh

I was aware that the notification settings being applied were from a previous page load, but didn't see the better option at the time.

Anyway, I'm glad your experienced eye saw the Vuex alternative.

Also, if rewriting these PRs is too time-consuming, feel free to leave advice or instructions on what to change, and I can assist with the modifications.

@joepavitt joepavitt merged commit 9bac4c2 into FlowFuse:main Dec 4, 2024
1 of 2 checks passed
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.

Ditch the Connection Re-established popup!
2 participants