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.

Sign up for GitHub

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

DT-617: Hide spinner after loading failure #2704

Closed
wants to merge 1 commit into from
Closed

DT-617: Hide spinner after loading failure #2704

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/pages/DatasetSearch.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ export const DatasetSearch = (props) => {
});
} catch (error) {
Notifications.showError({ text: 'Failed to load Elasticsearch index' });
setLoading(false);
Copy link
Contributor

@fboulnois fboulnois Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this is desirable, since it shows an empty table which may be misleading when on a custom data library. This means that we will not be able to tell the difference between a data table that has no data vs a data library that failed to load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true - the table doesn't know why there are no datasets, but it's also technically true that loading is complete. I noted in the PR description that I'm purposefully choosing not to try and modify the logic in the display to include an error state. If we were to go down that path, this would a larger refactor that I would prefer to do in a ticket with clear requirements in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a user I cannot take an action if the index fails to load, that's why I don't think it's desirable to go from a loading state (where it is clear that I cannot take an action) to an empty table (where it is unclear if I can take any actions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a user I cannot take an action if the index fails to load, that's why I don't think it's desirable to go from a loading state (where it is clear that I cannot take an action) to an empty table (where it is unclear if I can take any actions)

IIUC, is having the loading spinner remain visible in the error state the intended functionality? If so, I'll be happy to close this and close out the ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

Until we have a more defined approach to dealing with index failures (perhaps replacing the spinner with an error icon?), the spinner-as-error was the intended failure mode.

}
}
};
Expand Down
Loading