-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
App reload in cluster settings #858
Conversation
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
path: [clusterViewRoute.path, clusterSettingsRoute.path].flat(), | ||
exact: true | ||
}) | ||
const isActive = activeClusterId === cluster.id && !!matched; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to check route matching to highlight cluster icon?
const isActive = cluster.id === clusterStore.activeClusterId
not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, in case of using matchPath
should be like const isActive = cluster.id === matched?.params.clusterId
in your case.. but I suggest to remove it if no objections..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made suggested changes and completely removed matchPath
.
Apply whole
webContents
reload in Cluster Settings.Resolves #796
Signed-off-by: Alex Andreev alex.andreev.email@gmail.com