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

Fix #25805: Containers Portlet only show containers from current site #25966

Merged

Conversation

zJaaal
Copy link
Contributor

@zJaaal zJaaal commented Aug 31, 2023

πŸ€– Generated by Copilot at d087b61

Summary

πŸŽ¨πŸš€βœ…

Refactored the container list component to use the p-table component and the DotContainerListStore service for better UI and performance. Fixed a z-index issue with the content type selector component. Added a method and a test case for resetting extra parameters in the paginator service.

We're breaking free from the old data table
We're using the p-table to unleash our power
We're refactoring the code with the DotContainerListStore
We're fixing the bugs and improving the performance

Walkthrough

  • Replace dot-listing-data-table component with p-table component for better performance and flexibility (link, link, link, link, link, link, link, link, link, link, link)
  • Move dot-content-type-selector component and div with the container-listing__header-options class to the dot-action-header component for common header actions (link, link, link)
  • Add container-list component styles for the layout and responsiveness of the p-table component (link, link)
  • Add DotContainerListStore service to manage the state and effects of the containers list (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Add ElementRef, QueryList, ViewChild, ViewChildren and Menu imports for the Angular decorators and types for the container-list component and the p-table component (link)
  • Add InputTextModule and TableModule imports for the modules needed for the p-table component and the query input (link, link, link)
  • Add ActionHeaderModule import for the module needed for the dot-action-header component (link, link)
  • Add actionsMenu, tableRows, selectedContainers and destroy$ properties for the references and variables for the container-list component and the p-table component (link)
  • Add handleArchivedFilter, handleQueryFilter, loadDataPaginationEvent, handleActionMenuOpen and focusFirstRow methods for the query filter, the pagination, the action menu and the keyboard navigation for the p-table component (link)
  • Add appendTo="body" attribute to the p-dropdown element to fix a z-index issue with the dropdown menu (link)
  • Add resetExtraParams method to the PaginatorService class to remove all extra parameters of the eventual request (link, link)
  • Remove DotRouterService import from the container-list component since it is no longer needed with the DotContainerListStore service (link)
  • Remove columnsMock variable from the container-list component spec since it is no longer needed with the p-table component (link)
  • Remove getContainersWithDisabledEntities method from the container-list component since it is no longer needed with the DotContainerListStore service (link)

Screenshots

Screen.Recording.2023-09-01.at.1.04.54.PM.mov

@zJaaal zJaaal linked an issue Aug 31, 2023 that may be closed by this pull request
@zJaaal zJaaal marked this pull request as draft August 31, 2023 14:40
@zJaaal zJaaal requested a review from fmontes September 1, 2023 16:18
@zJaaal zJaaal marked this pull request as ready for review September 1, 2023 16:19
…t-site' of https://github.com/dotCMS/core into issue-25805-containers-only-show-containers-from-current-site
@zJaaal zJaaal requested a review from oidacra September 5, 2023 15:45
@dotcms-sonarqube
Copy link

private dotContainersService: DotContainersService
private dotContainersService: DotContainersService,
private paginatorService: PaginatorService,
private dotSiteService: SiteService
) {
super(null);
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +88 to +91
([[isEnterprise, hasEnvironments], containers]: [
[boolean, boolean],
DotContainer[]
]) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is too complicated, we can make this an object like:

{
  isEnterprise: boolean,
  hasEnvironments: boolean,
  containers: DotContainer[]
}

We might have to modify the resolver... but if you ask me with this approach the resolver is not doing anything.

* @memberof DotContainerListStore
*/
private patchContainers(containers: DotContainer[]): void {
this.patchState({
Copy link
Member

Choose a reason for hiding this comment

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

Why patch instead of set?

</div>
</dot-action-header>
<p-table
*ngIf="vm.tableColumns && vm.actionHeaderOptions"
Copy link
Member

Choose a reason for hiding this comment

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

Why two conditions here?

Comment on lines +8 to +12
dot-portlet-box {
display: flex;
flex-direction: column;
overflow-y: auto;
}
Copy link
Member

Choose a reason for hiding this comment

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

dot-portlet-box supposed to work without any modifications... should we make this global?

Comment on lines +20 to +24
display: flex;
flex-direction: column;
height: 100%;
justify-content: space-between;
background: $white;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be global?

@fmontes fmontes merged commit da4bce8 into master Sep 7, 2023
19 checks passed
@fmontes fmontes deleted the issue-25805-containers-only-show-containers-from-current-site branch September 7, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants