From 0ee376069680ef6d2188a18986b34c24920a9bbc Mon Sep 17 00:00:00 2001 From: Partha Aji Date: Tue, 17 Oct 2023 18:11:41 -0400 Subject: [PATCH] Refs #36793 - Refactored based on suggestions Apply suggestions from code review Co-authored-by: Jeremy Lenz --- app/registries/foreman/settings/general.rb | 4 +- .../components/HostsIndex/Selectors.js | 4 + .../react_app/components/HostsIndex/index.js | 108 +++++++++++++++-- .../PF4/TableIndexPage/ActionButtons.js | 59 ++++++--- .../PF4/TableIndexPage/ActionKebab.js | 26 +--- .../TableIndexPage/Table/SelectAllCheckbox.js | 4 +- .../Table/SelectAllCheckbox.scss | 2 +- .../PF4/TableIndexPage/Table/Table.js | 32 ++--- .../PF4/TableIndexPage/Table/TableHooks.js | 7 +- .../PF4/TableIndexPage/TableIndexPage.js | 113 +++--------------- .../routes/Models/ModelsPage/index.js | 10 ++ 11 files changed, 189 insertions(+), 180 deletions(-) create mode 100644 webpack/assets/javascripts/react_app/components/HostsIndex/Selectors.js diff --git a/app/registries/foreman/settings/general.rb b/app/registries/foreman/settings/general.rb index 464c1649093c..aa55a9865cf3 100644 --- a/app/registries/foreman/settings/general.rb +++ b/app/registries/foreman/settings/general.rb @@ -53,9 +53,9 @@ full_name: N_('Show Experimental Labs')) setting('new_hosts_pages', type: :boolean, - description: N_("Whether or not to show the new hosts page for All Hosts (requires reload of page)"), + description: N_("Whether or not to show the new overview page for All Hosts (requires reload of page)"), default: false, - full_name: N_('Show New Hosts Pages')) + full_name: N_('Show New Host Overview Page')) setting('display_fqdn_for_hosts', type: :boolean, description: N_('Display names of hosts as FQDNs. If disabled, only display names of hosts as hostnames.'), diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/Selectors.js b/webpack/assets/javascripts/react_app/components/HostsIndex/Selectors.js new file mode 100644 index 000000000000..69348e06fb1e --- /dev/null +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/Selectors.js @@ -0,0 +1,4 @@ +import { selectComponentByWeight } from '../common/Slot/SlotSelectors'; + +export const selectKebabItems = () => + selectComponentByWeight('hosts-index-kebab'); diff --git a/webpack/assets/javascripts/react_app/components/HostsIndex/index.js b/webpack/assets/javascripts/react_app/components/HostsIndex/index.js index e318b2c4cda9..292bb105b5d4 100644 --- a/webpack/assets/javascripts/react_app/components/HostsIndex/index.js +++ b/webpack/assets/javascripts/react_app/components/HostsIndex/index.js @@ -1,28 +1,107 @@ -import React from 'react'; -import { CubeIcon } from '@patternfly/react-icons'; +import React, { createContext } from 'react'; +import PropTypes from 'prop-types'; +import { useSelector, shallowEqual } from 'react-redux'; +import { Td } from '@patternfly/react-table'; +import { ToolbarItem } from '@patternfly/react-core'; import { translate as __ } from '../../common/I18n'; import TableIndexPage from '../PF4/TableIndexPage/TableIndexPage'; +import { ActionKebab } from '../PF4/TableIndexPage/ActionKebab'; import { HOSTS_API_PATH, API_REQUEST_KEY } from '../../routes/Hosts/constants'; +import { selectKebabItems } from './Selectors'; +import { useAPI } from '../../common/hooks/API/APIHooks'; +import { useBulkSelect } from '../PF4/TableIndexPage/Table/TableHooks'; +import SelectAllCheckbox from '../PF4/TableIndexPage/Table/SelectAllCheckbox'; +import { getPageStats } from '../PF4/TableIndexPage/Table/helpers'; + +export const ForemanHostsIndexActionsBarContext = createContext({}); const HostsIndex = () => { const columns = { name: { title: __('Name'), - wrapper: ({ id, name }) => ({name}), + wrapper: ({ id, name }) => {name}, isSorted: true, }, }; + const defaultParams = { search: '' }; // search || - const computeContentSource = search => - `/change_host_content_source?search=${search}`; + const response = useAPI('get', `${HOSTS_API_PATH}?include_permissions=true`, { + key: API_REQUEST_KEY, + params: defaultParams, + }); - const customActionKebabs = [ - { - title: __('Change content source'), - icon: , - computeHref: computeContentSource, + const { + response: { + search: apiSearchQuery, + results, + total, + per_page: perPage, + page, }, - ]; + } = response; + + const { pageRowCount } = getPageStats({ total, page, perPage }); + + const { fetchBulkParams, ...selectAllOptions } = useBulkSelect({ + results, + metadata: {}, + initialSearchQuery: apiSearchQuery || '', + }); + + const { + selectAll, + selectPage, + selectNone, + selectedCount, + selectOne, + areAllRowsOnPageSelected, + areAllRowsSelected, + isSelected, + } = selectAllOptions; + + const selectionToolbar = ( + + + + ); + + const RowSelectTd = ({ rowData }) => ( + { + selectOne(isSelecting, rowData.id); + }, + isSelected: isSelected(rowData.id), + disable: false, + }} + /> + ); + + RowSelectTd.propTypes = { + rowData: PropTypes.object.isRequired, + }; + + const actionNode = []; + const registeredItems = useSelector(selectKebabItems, shallowEqual); + const customToolbarItems = ( + + + + ); return ( { controller="hosts" isDeleteable columns={columns} - displaySelectAllCheckbox - customActionKebabs={customActionKebabs} creatable={false} + response={response} + customToolbarItems={customToolbarItems} + selectionToolbar={selectionToolbar} + showCheckboxes + rowSelectTd={RowSelectTd} /> ); }; diff --git a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionButtons.js b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionButtons.js index fe4c287ca79c..c285d27d042c 100644 --- a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionButtons.js +++ b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionButtons.js @@ -1,6 +1,11 @@ -import React from 'react'; +import React, { useState } from 'react'; import PropTypes from 'prop-types'; -import { Button } from '@patternfly/react-core'; +import { + Button, + Dropdown, + KebabToggle, + DropdownItem, +} from '@patternfly/react-core'; /** * Generate a button or a dropdown of buttons @@ -10,20 +15,43 @@ import { Button } from '@patternfly/react-core'; */ export const ActionButtons = ({ buttons: originalButtons }) => { const buttons = [...originalButtons]; + const [isOpen, setIsOpen] = useState(false); if (!buttons.length) return null; - - const pfButtons = buttons.map(button => ( - - )); - - return <>{pfButtons}; + const firstButton = buttons.shift(); + return ( + <> + + {buttons.length > 0 && ( + + } + isOpen={isOpen} + isPlain + dropdownItems={buttons.map(button => ( + + {button.icon} {button.title} + + ))} + /> + )} + + ); }; ActionButtons.propTypes = { @@ -32,7 +60,6 @@ ActionButtons.propTypes = { action: PropTypes.object, title: PropTypes.string, icon: PropTypes.node, - isDisabled: PropTypes.bool, }) ), }; diff --git a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionKebab.js b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionKebab.js index 0238acf399fc..0af1524b4679 100644 --- a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionKebab.js +++ b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/ActionKebab.js @@ -1,6 +1,6 @@ import React, { useState } from 'react'; import PropTypes from 'prop-types'; -import { Dropdown, KebabToggle, DropdownItem } from '@patternfly/react-core'; +import { Dropdown, KebabToggle } from '@patternfly/react-core'; /** * Generate a button or a dropdown of buttons @@ -8,8 +8,7 @@ import { Dropdown, KebabToggle, DropdownItem } from '@patternfly/react-core'; * @param {Object} action action to preform when the button is click can be href with data-method or Onclick * @return {Function} button component or splitbutton component */ -export const ActionKebab = ({ items: originalItems }) => { - const items = [...originalItems]; +export const ActionKebab = ({ items }) => { const [isOpen, setIsOpen] = useState(false); if (!items.length) return null; return ( @@ -25,17 +24,7 @@ export const ActionKebab = ({ items: originalItems }) => { } isOpen={isOpen} isPlain - dropdownItems={items.map(item => ( - - {item.icon} {item.title} - - ))} + dropdownItems={items} /> )} @@ -43,14 +32,7 @@ export const ActionKebab = ({ items: originalItems }) => { }; ActionKebab.propTypes = { - items: PropTypes.arrayOf( - PropTypes.shape({ - action: PropTypes.object, - title: PropTypes.string, - icon: PropTypes.node, - isDisabled: PropTypes.bool, - }) - ), + items: PropTypes.arrayOf(PropTypes.node), }; ActionKebab.defaultProps = { diff --git a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.js b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.js index bcc9911eb454..b63f23695993 100644 --- a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.js +++ b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.js @@ -115,8 +115,8 @@ const SelectAllCheckbox = ({ ouiaId="select-all-checkbox-dropdown-toggle" splitButtonItems={[ onSelectAllCheckboxChange(checked)} diff --git a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.scss b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.scss index 1e7e890bf505..171af1dab093 100644 --- a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.scss +++ b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/SelectAllCheckbox.scss @@ -1,3 +1,3 @@ -.tablewrapper-select-all-checkbox { +.table-select-all-checkbox { font-weight: normal; } diff --git a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/Table.js b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/Table.js index 2b511c6b4efa..1e13d82e6679 100644 --- a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/Table.js +++ b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/Table.js @@ -29,9 +29,8 @@ export const Table = ({ url, isPending, isEmbedded, - displaySelectAllCheckbox, - isSelected, - selectOne, + showCheckboxes, + rowSelectTd, }) => { const columnsToSortParams = {}; Object.keys(columns).forEach(key => { @@ -73,7 +72,7 @@ export const Table = ({ getActions && getActions({ id, name, ...item }), ].filter(Boolean); const columnNamesKeys = Object.keys(columns); - + const RowSelectTd = rowSelectTd; return ( <> - {displaySelectAllCheckbox && } + {showCheckboxes && } {columnNamesKeys.map(k => ( ( - {displaySelectAllCheckbox && ( - { - selectOne(isSelecting, result.id); - }, - isSelected: isSelected(result.id), - disable: false, - }} - /> - )} + {showCheckboxes && } {columnNamesKeys.map(k => ( {columns[k].wrapper ? columns[k].wrapper(result) : result[k]} @@ -183,9 +171,8 @@ Table.propTypes = { url: PropTypes.string.isRequired, isPending: PropTypes.bool.isRequired, isEmbedded: PropTypes.bool, - displaySelectAllCheckbox: PropTypes.bool, - isSelected: PropTypes.func, - selectOne: PropTypes.func, + rowSelectTd: PropTypes.func, + showCheckboxes: PropTypes.bool, }; Table.defaultProps = { @@ -195,7 +182,6 @@ Table.defaultProps = { getActions: null, results: [], isEmbedded: false, - displaySelectAllCheckbox: false, - selectOne: noop, - isSelected: noop, + rowSelectTd: noop, + showCheckboxes: false, }; diff --git a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/TableHooks.js b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/TableHooks.js index 20c2ad30d276..a5506cea1754 100644 --- a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/TableHooks.js +++ b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/Table/TableHooks.js @@ -215,7 +215,6 @@ export const useBulkSelect = ({ }; const selectAll = checked => { - console.log({ checked }); setSelectAllMode(checked); if (checked) { exclusionSet.clear(); @@ -225,7 +224,6 @@ export const useBulkSelect = ({ }; const fetchBulkParams = (idColumnName = idColumn) => { - console.log('fetchBulkParams'); const searchQueryWithExclusionSet = () => { const query = [ searchQuery, @@ -233,7 +231,6 @@ export const useBulkSelect = ({ !isEmpty(exclusionSet) && `${idColumnName} !^ (${[...exclusionSet].join(',')})`, ]; - console.log(query.filter(item => item).join(' and ')); return query.filter(item => item).join(' and '); }; @@ -242,8 +239,6 @@ export const useBulkSelect = ({ throw new Error('Cannot build a search query with no items selected'); return `${idColumnName} ^ (${[...inclusionSet].join(',')})`; }; - - console.log({ selectAllMode }); return selectAllMode ? searchQueryWithExclusionSet() : searchQueryWithInclusionSet(); @@ -299,4 +294,4 @@ export const useUrlParams = () => { searchParam, ...urlParams, }; -}; \ No newline at end of file +}; diff --git a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/TableIndexPage.js b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/TableIndexPage.js index 7a1d7f69613a..d7ec6dc8fc06 100644 --- a/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/TableIndexPage.js +++ b/webpack/assets/javascripts/react_app/components/PF4/TableIndexPage/TableIndexPage.js @@ -15,7 +15,6 @@ import { Text, PaginationVariant, } from '@patternfly/react-core'; -import Pagination from '../../Pagination'; import { createURL, @@ -24,19 +23,15 @@ import { getURIsearch, } from '../../../common/urlHelpers'; import { translate as __ } from '../../../common/I18n'; - -import { useAPI } from '../../../common/hooks/API/APIHooks'; +import { noop } from '../../../common/helpers'; +import Pagination from '../../Pagination'; import { getControllerSearchProps, STATUS } from '../../../constants'; import BreadcrumbBar from '../../BreadcrumbBar'; import SearchBar from '../../SearchBar'; import Head from '../../Head'; import { ActionButtons } from './ActionButtons'; -import { ActionKebab } from './ActionKebab'; import './TableIndexPage.scss'; import { Table } from './Table/Table'; -import { useBulkSelect } from './Table/TableHooks'; -import SelectAllCheckbox from './Table/SelectAllCheckbox'; -import { getPageStats } from './Table/helpers'; /** A page component that displays a table with data fetched from an API. It provides search and filtering functionality, and the ability to create new entries and export data. @@ -73,7 +68,6 @@ const TableIndexPage = ({ controller, creatable, customActionButtons, - customActionKebabs, customCreateAction, customExportURL, customHelpURL, @@ -85,7 +79,10 @@ const TableIndexPage = ({ isDeleteable, searchable, children, - displaySelectAllCheckbox, + selectionToolbar, + response, + showCheckboxes, + rowSelectTd, }) => { const history = useHistory(); const { location: { search: historySearch } = {} } = history || {}; @@ -115,39 +112,12 @@ const TableIndexPage = ({ }, status = STATUS.PENDING, setAPIOptions, - } = useAPI( - 'get', - apiUrl.includes('include_permissions') - ? apiUrl - : `${apiUrl}?include_permissions=true`, - { - ...apiOptions, - params: defaultParams, - } - ); - const { pageRowCount } = getPageStats({ total, page, perPage }); - const { - updateSearchQuery, - fetchBulkParams, - ...selectAllOptions - } = useBulkSelect({ - results, - metadata: {}, - }); + } = response; const onPagination = newPagination => { setParamsAndAPI({ ...params, ...newPagination }); }; - const { - selectAll, - selectPage, - selectNone, - selectedCount, - areAllRowsOnPageSelected, - areAllRowsSelected, - } = selectAllOptions; - const memoDefaultSearchProps = useMemo( () => getControllerSearchProps(controller), [controller] @@ -176,33 +146,6 @@ const TableIndexPage = ({ } }; - const processCustomElementActions = buttons => - buttons.map(button => { - const responseButton = { ...button }; - - if (selectedCount === 0) { - responseButton.isDisabled = true; - } - - if ( - displaySelectAllCheckbox && - responseButton.computeHref && - selectedCount > 0 - ) { - responseButton.action = { - href: responseButton.computeHref(fetchBulkParams()), - }; - } - return responseButton; - }); - - const additionalActionButtons = processCustomElementActions( - customActionButtons - ); - const additionalActionKebabs = processCustomElementActions( - customActionKebabs - ); - const actionButtons = [ creatable && canCreate && { @@ -220,7 +163,7 @@ const TableIndexPage = ({ icon: , action: { href: customHelpURL || helpURL() }, }, - ...additionalActionButtons, + ...customActionButtons, ].filter(item => item); return ( @@ -246,22 +189,7 @@ const TableIndexPage = ({ {searchable && ( - {displaySelectAllCheckbox && ( - - - - )} + {selectionToolbar} )} - {additionalActionKebabs.length > 0 && ( - - - - - - )} - {customToolbarItems && ( {customToolbarItems} )} @@ -328,8 +248,8 @@ const TableIndexPage = ({ status === STATUS.ERROR && errorMessage ? errorMessage : null } isPending={status === STATUS.PENDING} - {...selectAllOptions} - displaySelectAllCheckbox={displaySelectAllCheckbox} + showCheckboxes={showCheckboxes} + rowSelectTd={rowSelectTd} /> )} @@ -369,19 +289,21 @@ TableIndexPage.propTypes = { controller: PropTypes.string, creatable: PropTypes.bool, customActionButtons: PropTypes.array, - customActionKebabs: PropTypes.array, customCreateAction: PropTypes.func, customExportURL: PropTypes.string, customHelpURL: PropTypes.string, customSearchProps: PropTypes.object, customToolbarItems: PropTypes.node, + response: PropTypes.object.isRequired, exportable: PropTypes.bool, hasHelpPage: PropTypes.bool, header: PropTypes.string, isDeleteable: PropTypes.bool, searchable: PropTypes.bool, children: PropTypes.node, - displaySelectAllCheckbox: PropTypes.bool, + selectionToolbar: PropTypes.node, + rowSelectTd: PropTypes.func, + showCheckboxes: PropTypes.bool, }; TableIndexPage.defaultProps = { @@ -393,7 +315,6 @@ TableIndexPage.defaultProps = { controller: '', creatable: true, customActionButtons: [], - customActionKebabs: [], customCreateAction: null, customExportURL: '', customHelpURL: '', @@ -404,7 +325,9 @@ TableIndexPage.defaultProps = { header: '', isDeleteable: false, searchable: true, - displaySelectAllCheckbox: false, + selectionToolbar: null, + rowSelectTd: noop, + showCheckboxes: false, }; export default TableIndexPage; diff --git a/webpack/assets/javascripts/react_app/routes/Models/ModelsPage/index.js b/webpack/assets/javascripts/react_app/routes/Models/ModelsPage/index.js index a10faae2ae50..b46e278a8268 100644 --- a/webpack/assets/javascripts/react_app/routes/Models/ModelsPage/index.js +++ b/webpack/assets/javascripts/react_app/routes/Models/ModelsPage/index.js @@ -3,6 +3,7 @@ import React from 'react'; import { translate as __ } from '../../../common/I18n'; import TableIndexPage from '../../../components/PF4/TableIndexPage/TableIndexPage'; import { MODELS_API_PATH, API_REQUEST_KEY } from '../constants'; +import { useAPI } from '../../../common/hooks/API/APIHooks'; const ModelsPage = () => { const columns = { @@ -26,6 +27,14 @@ const ModelsPage = () => { title: __('Hosts'), }, }; + const response = useAPI( + 'get', + `${MODELS_API_PATH}?include_permissions=true`, + { + key: API_REQUEST_KEY, + } + ); + return ( { controller="models" isDeleteable columns={columns} + response={response} /> ); };