From 30253d21fcccc920d8696f8bbf7592dccae2cb59 Mon Sep 17 00:00:00 2001 From: John Mitchell Date: Tue, 30 Jul 2019 13:57:16 -0400 Subject: [PATCH] assorted ui_next search phase 1 pr feedback updates - remove unnecessary displayAll prop from ChipGroup - update notification api fn to be 2 with no boolean param - fix params passing to api functions --- awx/ui_next/SEARCH.md | 2 - .../src/api/mixins/Notifications.mixin.js | 53 ++++++++++++------- awx/ui_next/src/components/Chip/ChipGroup.jsx | 16 ++---- .../src/components/FilterTags/FilterTags.jsx | 3 +- .../PaginatedDataList/PaginatedDataList.jsx | 4 +- .../OrganizationAccess/OrganizationAccess.jsx | 2 +- .../OrganizationAccessItem.test.jsx.snap | 1 - .../OrganizationNotifications.jsx | 19 ++++--- .../OrganizationNotifications.test.jsx | 24 +++++---- awx/ui_next/src/util/qs.js | 5 +- awx/ui_next/src/util/qs.test.js | 26 +++++++-- awx/ui_next/testUtils/apiReusable.jsx | 35 ++++++++---- 12 files changed, 117 insertions(+), 73 deletions(-) diff --git a/awx/ui_next/SEARCH.md b/awx/ui_next/SEARCH.md index 0035213446..0e223b0fe8 100644 --- a/awx/ui_next/SEARCH.md +++ b/awx/ui_next/SEARCH.md @@ -108,8 +108,6 @@ Similar to the way the list grabs data based on changes to the react-router para Currently the filter tags do not display the key, though that data is available and they could very easily do so. -Some small changes were made to our custom ChipGroup component to make this possible--a new displayAll bool prop which bypasses all chip hiding based on number of chips. - ## QS Updates (and supporting duplicate keys) The logic that was updated to handle search tags can be found in the qs.js util file. diff --git a/awx/ui_next/src/api/mixins/Notifications.mixin.js b/awx/ui_next/src/api/mixins/Notifications.mixin.js index e40b7983a3..9625c29e04 100644 --- a/awx/ui_next/src/api/mixins/Notifications.mixin.js +++ b/awx/ui_next/src/api/mixins/Notifications.mixin.js @@ -14,14 +14,14 @@ const NotificationsMixin = parent => readNotificationTemplatesSuccess(id, params) { return this.http.get( `${this.baseUrl}${id}/notification_templates_success/`, - params + { params } ); } readNotificationTemplatesError(id, params) { return this.http.get( `${this.baseUrl}${id}/notification_templates_error/`, - params + { params } ); } @@ -54,43 +54,58 @@ const NotificationsMixin = parent => } /** - * This is a helper method meant to simplify setting the "on" or "off" status of + * This is a helper method meant to simplify setting the "on" status of * a related notification. * * @param[resourceId] - id of the base resource * @param[notificationId] - id of the notification * @param[notificationType] - the type of notification, options are "success" and "error" - * @param[associationState] - Boolean for associating or disassociating, options are true or false */ - // eslint-disable-next-line max-len - updateNotificationTemplateAssociation( + associateNotificationTemplate( resourceId, notificationId, - notificationType, - associationState + notificationType ) { - if (notificationType === 'success' && associationState === true) { + if (notificationType === 'success') { return this.associateNotificationTemplatesSuccess( resourceId, notificationId ); } - if (notificationType === 'success' && associationState === false) { - return this.disassociateNotificationTemplatesSuccess( - resourceId, - notificationId - ); - } - - if (notificationType === 'error' && associationState === true) { + if (notificationType === 'error') { return this.associateNotificationTemplatesError( resourceId, notificationId ); } - if (notificationType === 'error' && associationState === false) { + throw new Error( + `Unsupported notificationType for association: ${notificationType}` + ); + } + + /** + * This is a helper method meant to simplify setting the "off" status of + * a related notification. + * + * @param[resourceId] - id of the base resource + * @param[notificationId] - id of the notification + * @param[notificationType] - the type of notification, options are "success" and "error" + */ + disassociateNotificationTemplate( + resourceId, + notificationId, + notificationType + ) { + if (notificationType === 'success') { + return this.disassociateNotificationTemplatesSuccess( + resourceId, + notificationId + ); + } + + if (notificationType === 'error') { return this.disassociateNotificationTemplatesError( resourceId, notificationId @@ -98,7 +113,7 @@ const NotificationsMixin = parent => } throw new Error( - `Unsupported notificationType, associationState combination: ${notificationType}, ${associationState}` + `Unsupported notificationType for disassociation: ${notificationType}` ); } }; diff --git a/awx/ui_next/src/components/Chip/ChipGroup.jsx b/awx/ui_next/src/components/Chip/ChipGroup.jsx index 8d6d6f0593..58ba8ac7e5 100644 --- a/awx/ui_next/src/components/Chip/ChipGroup.jsx +++ b/awx/ui_next/src/components/Chip/ChipGroup.jsx @@ -1,15 +1,9 @@ import React, { useState } from 'react'; -import { number, bool } from 'prop-types'; +import { number } from 'prop-types'; import styled from 'styled-components'; import Chip from './Chip'; -const ChipGroup = ({ - children, - className, - showOverflowAfter, - displayAll, - ...props -}) => { +const ChipGroup = ({ children, className, showOverflowAfter, ...props }) => { const [isExpanded, setIsExpanded] = useState(!showOverflowAfter); const toggleIsOpen = () => setIsExpanded(!isExpanded); @@ -26,8 +20,8 @@ const ChipGroup = ({ return (