From c5a4dd80f7953afcb52eb5bfc72efbd5f89ad82b Mon Sep 17 00:00:00 2001 From: kamre Date: Wed, 27 Oct 2021 14:04:15 +0700 Subject: [PATCH] [GH-705] Auto-name new property (#1408) * Open menu with property name and type when new property is added. * Adding new property asks for the type and sets initial name. * Component for property types menu items introduced. * Close property menu when enter is pressed in the input. * Unit test for `CardDetailProperties` fixed: - jest snapshot updated - switched from mocking `fetch` to mocking `mutator` * Unit tests for `CardDetailProperties` updated: - use the recommended way to get elements and trigger user events - test for properties menu when adding new property added * Unit tests for `CardDetailProperties` added: - delete existing property - add new property * Don't use debouncing for property type selection. * Fix unit test for deleting property. * Width of button `+ Add a property` is fixed. * Jest snapshot updated after merge with `main` --- .../__snapshots__/cardDialog.test.tsx.snap | 36 +- .../cardDetailProperties.test.tsx.snap | 378 +++++++++++++++++- .../src/components/cardDetail/cardDetail.scss | 5 - .../cardDetail/cardDetailProperties.test.tsx | 160 +++++--- .../cardDetail/cardDetailProperties.tsx | 57 ++- webapp/src/mutator.ts | 5 +- webapp/src/widgets/menuWrapper.tsx | 3 +- webapp/src/widgets/propertyMenu.tsx | 99 +++-- 8 files changed, 599 insertions(+), 144 deletions(-) diff --git a/webapp/src/components/__snapshots__/cardDialog.test.tsx.snap b/webapp/src/components/__snapshots__/cardDialog.test.tsx.snap index 9ef9c5c4e..437ed9577 100644 --- a/webapp/src/components/__snapshots__/cardDialog.test.tsx.snap +++ b/webapp/src/components/__snapshots__/cardDialog.test.tsx.snap @@ -287,13 +287,19 @@ exports[`components/cardDialog return cardDialog menu content 1`] = `
- + +
@@ -693,13 +699,19 @@ exports[`components/cardDialog should match snapshot 1`] = `
- + +
diff --git a/webapp/src/components/cardDetail/__snapshots__/cardDetailProperties.test.tsx.snap b/webapp/src/components/cardDetail/__snapshots__/cardDetailProperties.test.tsx.snap index cbb3e60e5..814a4f13f 100644 --- a/webapp/src/components/cardDetail/__snapshots__/cardDetailProperties.test.tsx.snap +++ b/webapp/src/components/cardDetail/__snapshots__/cardDetailProperties.test.tsx.snap @@ -45,14 +45,378 @@ exports[`components/cardDetail/CardDetailProperties should match snapshot 1`] =
- + +
+ + + +`; + +exports[`components/cardDetail/CardDetailProperties should show property types menu 1`] = ` +
+
+
+
+
+ +
+
+
+ + + Jean-Luc Picard + + +
+
+
+
+ +
+
+
+
+
+
+ + Select property type + +
+
+
+
+
+
+
+ Text +
+
+
+
+
+
+ Number +
+
+
+
+
+
+ Email +
+
+
+
+
+
+ Phone +
+
+
+
+
+
+ URL +
+
+
+
+
+
+ Select +
+
+
+
+
+
+ Multi Select +
+
+
+
+
+
+ Date +
+
+
+
+
+
+ Person +
+
+
+
+
+
+ Checkbox +
+
+
+
+
+
+ Created time +
+
+
+
+
+
+ Created by +
+
+
+
+
+
+ Last updated time +
+
+
+
+
+
+ Last updated by +
+
+
+
+
+
+
+
+
+ Cancel +
+
+
+
+
+
+
diff --git a/webapp/src/components/cardDetail/cardDetail.scss b/webapp/src/components/cardDetail/cardDetail.scss index b2be45050..6be784598 100644 --- a/webapp/src/components/cardDetail/cardDetail.scss +++ b/webapp/src/components/cardDetail/cardDetail.scss @@ -26,11 +26,6 @@ display: flex; flex-direction: column; width: 100%; - - .MenuWrapper { - position: relative; - align-self: center; - } } .octo-propertyrow { diff --git a/webapp/src/components/cardDetail/cardDetailProperties.test.tsx b/webapp/src/components/cardDetail/cardDetailProperties.test.tsx index b2bf81082..10a481033 100644 --- a/webapp/src/components/cardDetail/cardDetailProperties.test.tsx +++ b/webapp/src/components/cardDetail/cardDetailProperties.test.tsx @@ -2,23 +2,24 @@ // See LICENSE.txt for license information. import React from 'react' -import {fireEvent, render} from '@testing-library/react' +import {render, screen, act} from '@testing-library/react' import userEvent from '@testing-library/user-event' +import '@testing-library/jest-dom' +import {mocked} from 'ts-jest/utils' + +import {createIntl} from 'react-intl' import {wrapIntl} from '../../testUtils' - import {TestBlockFactory} from '../../test/testBlockFactory' -import {FetchMock} from '../../test/fetchMock' +import mutator from '../../mutator' +import {propertyTypesList, typeDisplayName} from '../../widgets/propertyMenu' -import 'isomorphic-fetch' +import {PropertyType} from '../../blocks/board' import CardDetailProperties from './cardDetailProperties' -global.fetch = FetchMock.fn - -beforeEach(() => { - FetchMock.fn.mockReset() -}) +jest.mock('../../mutator') +const mockedMutator = mocked(mutator, true) describe('components/cardDetail/CardDetailProperties', () => { const board = TestBlockFactory.createBoard() @@ -51,66 +52,109 @@ describe('components/cardDetail/CardDetailProperties', () => { view.fields.sortOptions = [] view.fields.groupById = undefined view.fields.hiddenOptionIds = [] + const views = [view] const card = TestBlockFactory.createCard(board) card.fields.properties.property_id_1 = 'property_value_id_1' + const cards = [card] - const cardTemplate = TestBlockFactory.createCard(board) - cardTemplate.fields.isTemplate = true + const cardDetailProps = { + board, + card, + cards, + contents: [], + comments: [], + activeView: view, + views, + readonly: false, + } - test('should match snapshot', async () => { - const component = wrapIntl(( - - )) - - const {container} = render(component) + it('should match snapshot', async () => { + const {container} = render( + wrapIntl( + , + ), + ) expect(container).toMatchSnapshot() }) - test('rename select property', async () => { - const component = wrapIntl(( - - )) + it('should rename existing select property', async () => { + render( + wrapIntl( + , + ), + ) - const {container} = render(component) - const propertyLabel = container.querySelector('.MenuWrapper') - expect(propertyLabel).toBeDefined() - fireEvent.click(propertyLabel!) + const menuElement = screen.getByRole('button', {name: 'Owner'}) + userEvent.click(menuElement) - const propertyNameInput = container.querySelector('.PropertyMenu.menu-textbox') - expect(propertyNameInput).toBeDefined() - userEvent.type(propertyNameInput!, 'Owner - Renamed{enter}') - fireEvent.click(propertyLabel!) + const newName = 'Owner - Renamed' + const propertyNameInput = screen.getByRole('textbox') + userEvent.type(propertyNameInput, `${newName}{enter}`) - // should be called once on renaming the property - expect(FetchMock.fn).toBeCalledTimes(1) + const propertyTemplate = board.fields.cardProperties[0] + expect(mockedMutator.changePropertyTypeAndName).toHaveBeenCalledTimes(1) + expect(mockedMutator.changePropertyTypeAndName).toHaveBeenCalledWith(board, cards, propertyTemplate, 'select', newName) + }) - // Verify API call was made with renamed property - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore - const lastAPICallPayload = JSON.parse(FetchMock.fn.mock.calls[0][1].body) - expect(lastAPICallPayload[0].fields.cardProperties[0].name).toBe('Owner - Renamed') - expect(lastAPICallPayload[0].fields.cardProperties[0].options.length).toBe(3) - expect(lastAPICallPayload[0].fields.cardProperties[0].options[0].value).toBe('Jean-Luc Picard') - expect(lastAPICallPayload[0].fields.cardProperties[0].options[1].value).toBe('William Riker') - expect(lastAPICallPayload[0].fields.cardProperties[0].options[2].value).toBe('Deanna Troi') + it('should show confirmation dialog when deleting existing select property', () => { + render( + wrapIntl( + , + ), + ) + + const menuElement = screen.getByRole('button', {name: 'Owner'}) + userEvent.click(menuElement) + + const deleteButton = screen.getByRole('button', {name: /delete/i}) + userEvent.click(deleteButton) + + expect(screen.getByRole('heading', {name: 'Confirm Delete Property'})).toBeInTheDocument() + expect(screen.getByRole('button', {name: /delete/i})).toBeInTheDocument() + }) + + it('should show property types menu', () => { + const intl = createIntl({locale: 'en'}) + const {container} = render( + wrapIntl( + , + ), + ) + + const menuElement = screen.getByRole('button', {name: /add a property/i}) + userEvent.click(menuElement) + expect(container).toMatchSnapshot() + + const selectProperty = screen.getByText(/select property type/i) + expect(selectProperty).toBeInTheDocument() + + propertyTypesList.forEach((type: PropertyType) => { + const typeButton = screen.getByRole('button', {name: typeDisplayName(intl, type)}) + expect(typeButton).toBeInTheDocument() + }) + }) + + it('should add new number property', async () => { + render( + wrapIntl( + , + ), + ) + + const menuElement = screen.getByRole('button', {name: /add a property/i}) + userEvent.click(menuElement) + + await act(async () => { + const numberType = screen.getByRole('button', {name: /number/i}) + userEvent.click(numberType) + }) + + expect(mockedMutator.insertPropertyTemplate).toHaveBeenCalledTimes(1) + const args = mockedMutator.insertPropertyTemplate.mock.calls[0] + const template = args[3] + expect(template).toBeTruthy() + expect(template!.name).toMatch(/number/i) + expect(template!.type).toBe('number') }) }) diff --git a/webapp/src/components/cardDetail/cardDetailProperties.tsx b/webapp/src/components/cardDetail/cardDetailProperties.tsx index cb9dc58b0..293c4d1a8 100644 --- a/webapp/src/components/cardDetail/cardDetailProperties.tsx +++ b/webapp/src/components/cardDetail/cardDetailProperties.tsx @@ -1,10 +1,9 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. +import React, {useEffect, useState} from 'react' +import {FormattedMessage, useIntl} from 'react-intl' -import React, {useState} from 'react' -import {useIntl, FormattedMessage} from 'react-intl' - -import {Board, PropertyType, IPropertyTemplate} from '../../blocks/board' +import {Board, IPropertyTemplate, PropertyType} from '../../blocks/board' import {Card} from '../../blocks/card' import {BoardView} from '../../blocks/boardView' import {ContentBlock} from '../../blocks/contentBlock' @@ -12,11 +11,13 @@ import {CommentBlock} from '../../blocks/commentBlock' import mutator from '../../mutator' import Button from '../../widgets/buttons/button' import MenuWrapper from '../../widgets/menuWrapper' -import PropertyMenu from '../../widgets/propertyMenu' +import PropertyMenu, {PropertyTypes, typeDisplayName} from '../../widgets/propertyMenu' import PropertyValueElement from '../propertyValueElement' import {ConfirmationDialogBox} from '../confirmationDialogBox' import {sendFlashMessage} from '../flashMessages' +import Menu from '../../widgets/menu' +import {IDType, Utils} from '../../utils' type Props = { board: Board @@ -30,8 +31,16 @@ type Props = { } const CardDetailProperties = React.memo((props: Props) => { - const intl = useIntl() const {board, card, cards, views, activeView, contents, comments} = props + const [newTemplateId, setNewTemplateId] = useState('') + const intl = useIntl() + + useEffect(() => { + const newProperty = board.fields.cardProperties.find((property) => property.id === newTemplateId) + if (newProperty) { + setNewTemplateId('') + } + }, [newTemplateId, board.fields.cardProperties]) const [showConfirmationDialog, setShowConfirmationDialog] = useState(false) const [deletingPropId, setDeletingPropId] = useState('') @@ -48,7 +57,7 @@ const CardDetailProperties = React.memo((props: Props) => { > {props.readonly &&
{propertyTemplate.name}
} {!props.readonly && - +
{ {!props.readonly &&
- + + + + { + const template: IPropertyTemplate = { + id: Utils.createGuid(IDType.BlockID), + name: typeDisplayName(intl, type), + type, + options: [], + } + const templateId = await mutator.insertPropertyTemplate(board, activeView, -1, template) + setNewTemplateId(templateId) + }} + /> + +
}
diff --git a/webapp/src/mutator.ts b/webapp/src/mutator.ts index be3e615c3..b0d4d59cb 100644 --- a/webapp/src/mutator.ts +++ b/webapp/src/mutator.ts @@ -224,10 +224,10 @@ class Mutator { // Property Templates - async insertPropertyTemplate(board: Board, activeView: BoardView, index = -1, template?: IPropertyTemplate) { + async insertPropertyTemplate(board: Board, activeView: BoardView, index = -1, template?: IPropertyTemplate): Promise { if (!activeView) { Utils.assertFailure('insertPropertyTemplate: no activeView') - return + return '' } const newTemplate = template || { @@ -257,6 +257,7 @@ class Mutator { } await this.updateBlocks(changedBlocks, oldBlocks, description) + return newTemplate.id } async duplicatePropertyTemplate(board: Board, activeView: BoardView, propertyId: string) { diff --git a/webapp/src/widgets/menuWrapper.tsx b/webapp/src/widgets/menuWrapper.tsx index cf116c30b..e53fbc630 100644 --- a/webapp/src/widgets/menuWrapper.tsx +++ b/webapp/src/widgets/menuWrapper.tsx @@ -10,11 +10,12 @@ type Props = { stopPropagationOnToggle?: boolean; className?: string disabled?: boolean + isOpen?: boolean } const MenuWrapper = React.memo((props: Props) => { const node = useRef(null) - const [open, setOpen] = useState(false) + const [open, setOpen] = useState(Boolean(props.isOpen)) if (!Array.isArray(props.children) || props.children.length !== 2) { throw new Error('MenuWrapper needs exactly 2 children') diff --git a/webapp/src/widgets/propertyMenu.tsx b/webapp/src/widgets/propertyMenu.tsx index 02892614b..9b18c4adf 100644 --- a/webapp/src/widgets/propertyMenu.tsx +++ b/webapp/src/widgets/propertyMenu.tsx @@ -1,7 +1,6 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. import React, {useState, useRef, useEffect} from 'react' -import debounce from 'lodash/debounce' import {useIntl, IntlShape} from 'react-intl' import {PropertyType} from '../blocks/board' @@ -17,7 +16,7 @@ type Props = { onDelete: (id: string) => void } -function typeDisplayName(intl: IntlShape, type: PropertyType): string { +export function typeDisplayName(intl: IntlShape, type: PropertyType): string { switch (type) { case 'text': return intl.formatMessage({id: 'PropertyType.Text', defaultMessage: 'Text'}) case 'number': return intl.formatMessage({id: 'PropertyType.Number', defaultMessage: 'Number'}) @@ -44,6 +43,52 @@ function typeMenuTitle(intl: IntlShape, type: PropertyType): string { return `${intl.formatMessage({id: 'PropertyMenu.typeTitle', defaultMessage: 'Type'})}: ${typeDisplayName(intl, type)}` } +export const propertyTypesList: PropertyType[] = [ + 'text', + 'number', + 'email', + 'phone', + 'url', + 'select', + 'multiSelect', + 'date', + 'person', + 'checkbox', + 'createdTime', + 'createdBy', + 'updatedTime', + 'updatedBy', +] + +type TypesProps = { + label: string + onTypeSelected: (type: PropertyType) => void +} + +export const PropertyTypes = (props: TypesProps): JSX.Element => { + const intl = useIntl() + return ( + <> + + {props.label} + + + + + { + propertyTypesList.map((type) => ( + props.onTypeSelected(type)} + /> + )) + } + + ) +} + const PropertyMenu = React.memo((props: Props) => { const intl = useIntl() const nameTextbox = useRef(null) @@ -54,30 +99,11 @@ const PropertyMenu = React.memo((props: Props) => { defaultMessage: 'Delete', }) - const debouncedOnTypeAndNameChanged = (newType: PropertyType) => debounce(() => props.onTypeAndNameChanged(newType, name), 150) - useEffect(() => { nameTextbox.current?.focus() nameTextbox.current?.setSelectionRange(0, name.length) }, []) - const propertyTypes = [ - {type: 'text'}, - {type: 'number'}, - {type: 'email'}, - {type: 'phone'}, - {type: 'url'}, - {type: 'select'}, - {type: 'multiSelect'}, - {type: 'date'}, - {type: 'person'}, - {type: 'checkbox'}, - {type: 'createdTime'}, - {type: 'createdBy'}, - {type: 'updatedTime'}, - {type: 'updatedBy'}, - ] - return ( { type='text' className='PropertyMenu menu-textbox' onClick={(e) => e.stopPropagation()} - onChange={(e) => setName(e.target.value)} + onChange={(e) => { + setName(e.target.value) + }} value={name} onBlur={() => props.onTypeAndNameChanged(props.propertyType, name)} onKeyDown={(e) => { - if (e.keyCode === 13 || e.keyCode === 27) { + if (e.key === 'Enter' || e.key === 'Escape') { props.onTypeAndNameChanged(props.propertyType, name) e.stopPropagation() + if (e.key === 'Enter') { + e.target.dispatchEvent(new Event('menuItemClicked')) + } } }} spellCheck={true} @@ -100,24 +131,10 @@ const PropertyMenu = React.memo((props: Props) => { id='type' name={typeMenuTitle(intl, props.propertyType)} > - - - {intl.formatMessage({id: 'PropertyMenu.changeType', defaultMessage: 'Change property type'})} - - - - - - { - propertyTypes.map((propertyType) => ( - debouncedOnTypeAndNameChanged(propertyType.type as PropertyType)()} - /> - )) - } + props.onTypeAndNameChanged(type, name)} + />