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

Keep up-to-date resource in the details view (Drawer) #7224

Merged
merged 7 commits into from
Mar 13, 2023

Conversation

ixrock
Copy link
Member

@ixrock ixrock commented Feb 23, 2023

Minimal alternative to #7187
Fix #7186

Screen.Recording.2023-02-17.at.16.24.33.mov

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock ixrock added the bug Something isn't working label Feb 23, 2023
@ixrock ixrock added this to the 6.5.0 milestone Feb 23, 2023
@ixrock ixrock requested a review from a team as a code owner February 23, 2023 11:41
@ixrock ixrock self-assigned this Feb 23, 2023
@ixrock ixrock requested review from jansav, Nokel81, aleksfront and Iku-turso and removed request for a team February 23, 2023 11:41
Nokel81
Nokel81 previously approved these changes Feb 23, 2023
Copy link
Collaborator

@Nokel81 Nokel81 left a comment

Choose a reason for hiding this comment

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

Certainly an improvement. Nice small find!

Signed-off-by: Roman <ixrock@gmail.com>
… wrong

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock ixrock force-pushed the fix/keep_fresh_resource_in_details_view branch from c01579c to b4e17f5 Compare February 24, 2023 14:10
aleksfront
aleksfront previously approved these changes Feb 28, 2023
@jim-docker
Copy link
Contributor

Certainly an improvement. Nice small find!

My favourite kind of fix, small and precise. @Nokel81 Could this make it for 6.4.0?

@@ -100,7 +103,8 @@ describe("reactively hide kube object detail item", () => {
expect(rendered.baseElement).toMatchSnapshot();
});

it("shows the kube object detail item", () => {
// FIXME: details not rendered in the Drawer (in snapshot?)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit worrisome that some tests no longer pass though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that is worrisome

@Nokel81
Copy link
Collaborator

Nokel81 commented Feb 28, 2023

Could this make it for 6.4.0?

Don't want to with those tests being skipped

@Nokel81
Copy link
Collaborator

Nokel81 commented Feb 28, 2023

Resource loading has failed:

TypeError: store.getByPath is not a function

This doesn't look right for a snapshot

Copy link
Collaborator

@Nokel81 Nokel81 left a comment

Choose a reason for hiding this comment

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

This shouldn't me merged until the snapshots are fixed.

The override probably needs to be updated

@@ -33,6 +41,8 @@ describe("reactively hide kube object detail item", () => {
const store = {
api,
loadFromPath: async () => getKubeObjectStub("some-kind", "some-api-version"),
getByPath() {
Copy link
Member Author

Choose a reason for hiding this comment

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

If I comment this line I'm getting very uninformative/misleading error description.. Can we improve that somehow?
Why there is nothing about that store.getByPath(path: string) not available?

Screenshot 2023-03-13 at 15 21 14

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the error is within a render call and is being swallowed by ErrorBoundary. If you look at the snapshot you will see the DOM with the error. I agree it is a bit annoying.

@ixrock
Copy link
Member Author

ixrock commented Mar 13, 2023

@Nokel81 tests are fixed, PTAL

@Nokel81 Nokel81 merged commit 79a4eb3 into master Mar 13, 2023
@Nokel81 Nokel81 deleted the fix/keep_fresh_resource_in_details_view branch March 13, 2023 14:45
@Nokel81 Nokel81 mentioned this pull request Mar 13, 2023
gabriel-mirantis pushed a commit that referenced this pull request Mar 21, 2023
* alternative to #7187

Signed-off-by: Roman <ixrock@gmail.com>

* update snapshots with `jest src -u` from `packages/core`

Signed-off-by: Roman <ixrock@gmail.com>

* skipping some tests cause i have no idea how to fix those and what is wrong

Signed-off-by: Roman <ixrock@gmail.com>

* fix tests

Signed-off-by: Roman <ixrock@gmail.com>

---------

Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Gabriel <gaccettola@mirantis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stale resource in details panel after update from apis/store
4 participants