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

Use DataCollection for all listings #2388

Closed
1 of 2 tasks
johncowen opened this issue Apr 8, 2024 · 1 comment · Fixed by #2764
Closed
1 of 2 tasks

Use DataCollection for all listings #2388

johncowen opened this issue Apr 8, 2024 · 1 comment · Fixed by #2764
Assignees
Labels
kind/cleanup Cleanup/refactor an existing component/code triage/accepted The issue was reviewed and is complete enough to start working on it
Milestone

Comments

@johncowen
Copy link
Contributor

johncowen commented Apr 8, 2024

Description

A little while back we added a DataCollection component (#2071) as a tool to manage 'collections' of data, filter and find on collections and show empty states for empty collections.

We've used this now in quite a few areas, both simple and more complex interactions whilst adding new features to the GUI. We should roll this out to the remaining places that haven't been updated as a consequence of/whilst adding new features.


Whilst DataCollection is already useful, there are a couple of things we may want to think about either before/during/after doing this (preferably not after but that wouldn't be a big deal either), mostly surrounding its default embedded EmptyBlock:

  1. Relatively often we need to overwrite the default EmptyBlock only because we want to put the "name of the thing" in the empty state message - i.e. "There are no Things"
  2. Occasionally we need to decide whether to show a [Create Button] for the collection inside the EmptyBlock, but in certain circumstances this can depend on whether you have permissions to do so or not.

Instead of having to always provide a custom empty state slot/component for the above things (which reduces the usefulness of DataCollection), we should consider adding say a :type attribute i.e. :type="dataplane". We can then use type within the component to check any permissions for the [Create Button] and also use it as a part of a i18n key.

Note: We made specific comment to "defaulting to avoiding" using t within reusable components, but I think this is one of the times I had in mind where the balance tips and its probably good idea to use t from within the EmptyState component itself:

t(`components.empty-state.${props.type}.message`, {}, {defaultMessage: t('components.empty-state.default.message')})

t within the component would be injected/injectable as per usual and we would use props.type for both i18n and permission checking. I'm almost sure we should use something like <DataSource src="/me/permissions/${prop.type}"> for checking the permission for whether to show the [Create Button] or not

Lastly as a tiny detail, EmptyBlock should be moved to XEmptyState as an eXtra eXtension over KEmptyState, and for the sake of consistency we should also move LoadingBlock and ErrorBlock to XLoadingState and XErrorState.

@johncowen johncowen added triage/pending This issue will be looked at on the next triage meeting kind/cleanup Cleanup/refactor an existing component/code labels Apr 8, 2024
@johncowen johncowen added this to the 2.8.x milestone Apr 8, 2024
@jakubdyszkiewicz jakubdyszkiewicz added triage/accepted The issue was reviewed and is complete enough to start working on it and removed triage/pending This issue will be looked at on the next triage meeting labels Apr 8, 2024
@lahabana lahabana modified the milestones: 2.8.x, 2.9.x May 21, 2024
@johncowen
Copy link
Contributor Author

johncowen commented Jun 26, 2024

Instead of having to always provide a custom empty state slot/component for the above things (which reduces the usefulness of DataCollection), we should consider adding say a :type attribute i.e. :type="dataplane". We can then use type within the component to check any permissions for the [Create Button] and also use it as a part of a i18n key.

Note: We made specific comment to "defaulting to avoiding" using t within reusable components, but I think this is one of the times I had in mind where the balance tips and its probably good idea to use t from within the EmptyState component itself:

t(`components.empty-state.${props.type}.message`, {}, {defaultMessage: t('components.empty-state.default.message')})

^ that part in the description has been done

johncowen added a commit that referenced this issue Jun 27, 2024
Part of #2388

This PR ended up doing several things:

- Uses DataCollection for the lists on the home/ control plane detail page (Zones and Meshes)
- Refactors the way that a `[+ Create Zone]` is conditionally added to the page. We now use a method which means we can decorate the `ZoneControlPlaneList` component from The Outside to add the button if we want to do that eventually. It's also generally nicer as we define the button once and then add to the DOM in the correct place, instead of defining twice and then wrapping both in slightly different conditionals.
- I noticed that the test we use for this mocks the mesh-insights API response incorrectly. So I amended that in the test and used a more generic method we have for mocking 'partitions of totals' in the mocks.

---------

Signed-off-by: John Cowen <[email protected]>
@johncowen johncowen self-assigned this Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Cleanup/refactor an existing component/code triage/accepted The issue was reviewed and is complete enough to start working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants