ui: agentless nodes banner #14971
ui: agentless nodes banner #14971
Conversation
WenInCode
force-pushed
the
ui/feature/agentless-nodes-banner
branch
from
9b8fe42
to
80d2694
Compare
WenInCode
force-pushed
the
ui/feature/agentless-nodes-banner
branch
from
80d2694
to
8a685a1
Compare
WenInCode
force-pushed
the
ui/feature/agentless-nodes-banner
branch
from
ca25c02
to
09514da
Compare
|
I think we want to show this per cluster since it would be different Kubernetes deployments, and it might be a good reminder. There's nowhere in the UI right now that explicitly calls out whether a cluster is a Kubernetes deployment or not, so users may (?) forget and wonder why there are no client agents if the banner has been removed. @jkirschner-hashicorp do you agree, or do you think showing it once is fine? |
|
@jnwright I've udpated it to save the key based on the datacenter, so the banner will show up per dc. If you want to try this out you can on this build: https://consul-ui-staging-ds2vj97yj-hashicorp.vercel.app/ui/dc1/nodes I'll work on getting the mock-data working (without breaking existing tests) after lunch in a new branch. |
|
is there a way to do it per partition? clusters are represented by partitions in ENT, not datacenters. I'm trying to consider the case of a user having more than one kubernetes deployment. So one or more partitions are kubernetes, and one is a non-agentless deployment. if they dismiss it once for the whole datacenter, they could switch to the other runtime's cluster and see client nodes. they would then switch to the other kubernetes partition and not remember that it's kubernetes. if there's no banner, they could think it's an error that there are no client nodes. |
|
@jnwright I did not know that's how it worked for ENT. I've updated it to have a postfix that is a combination of both dc and partition. so if one of them is present it will be that one, but if both are present it will be both. you can try it out here: https://consul-ui-staging-2c89cdyf6-hashicorp.vercel.app/ui/dc1/nodes |
ui/packages/consul-ui/app/components/consul/node/agentless-notice/index.js
Outdated
Show resolved
Hide resolved
| this.storageKey = `consul-nodes-agentless-notice-dismissed-${this.args.postfix}`; | ||
| } | ||
|
|
||
| if (window.localStorage.getItem(this.storageKey) === DISMISSED_VALUE) { |
There was a problem hiding this comment.
Three things that came to my mind when looking at this:
- Accessing local-storage directly and not wrapping it will make the test-suite really persist values when we dismiss a notice
- We are checking against a string, although we are setting a boolean when dismissing
- We aren't really deriving state, but checking local storage and then setting state based on that, which kind of goes against the reactive state ideas of Octane.
What I could imagine doing is creating a wrapper service for local-storage that works with storage classes that encapsulate the buckets of storage we want to interact with. This sounds complicated and may be overkill for a use-case like this, but I suppose we have other places in the application that could benefit from an approach like this.
I prototyped a mechanism that is heavily inspired by ember-local-storage, but works with tracked properties in a repository of mine:
https://github.com/LevelbossMike/local-storage-service
I also demonstrate how this mechanism could be used in testing so that we don't pollute local-storage when executing the test-suite.
Having said that, I'm unsure, if it would be worth the effort to add a mechanism like this for this particular feature. How do you feel about this?
There was a problem hiding this comment.
@LevelbossMike I originally was trying to use ember-tracked-local-storage to get the reactive nature of it, instead of reading that localstorage state and setting local state based on it. I ran into issues with setting up dynamic keys with the tracked property. However, that doesn't look like it would be an issue in your package.
Ember question for you. I don't see where the notices get tracked in your example. Do changes to services state trigger changes in the same way that arguments or tracked properties do?
There was a problem hiding this comment.
Just leaving this here for history. As discussed briefly in a call: In the example repository, I'm setting up the notices-storage so that it uses a tracked array internally. That's the reason we can create reactive getters based on the state-property of the notices-storage.
Reference: https://github.com/tracked-tools/tracked-built-ins
Use local-storage service, prototyped here https://github.com/LevelbossMike/local-storage-service, to manage local storage usage in an octane way. Does not write to local storage in tests by default and is easy to stub out.
|
@LevelbossMike letting you know it's ready for another review from you tomorrow |
Description
Displays a notice banner on the nodes index page if there are agentless nodes that are being filtered. I've adopted the existing
<Notice>component styling, instead of exactly what was prescribed in the designs. Should we want to update the<Notice>banner design, I would be happy to do so in a follow-up.I discussed how we should be storing the dismissal in consul in the tech-frontend channel. There is not a good way for consul to persist this data at the moment, and because we have anonymous users, the key can't be user based. For now I am just storing this key in local storage.
Right now, if you were to dismiss the notice it would not show up again on any datacenter. Should we want to have it show up per datacenter, we could add that to the key.This is now being saved per dc.Updated Demo:
https://www.loom.com/share/311578f76ac2406f8be4c38d0fab3a8e
Screenshots
Links
PR Checklist