Skip to content

feat(connect-form): auto add advanced options and remove button #2738

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 1, 2022
Merged

feat(connect-form): auto add advanced options and remove button #2738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03e7403
feat(connect-form): auto add advanced options and remove button
mcasimir Jan 31, 2022
463d05a
remove cleanup
mcasimir Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/connect-form/src/components/advanced-options-tabs/advanced-tab/advanced-tab.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ function AdvancedTab({
/>
</FormFieldContainer>
{/* Default Database */}

<FormFieldContainer>
<TextInput
spellCheck={false}
Expand All @@ -137,12 +138,12 @@ function AdvancedTab({
}}
name={'default-database'}
data-testid={'default-database'}
label={'Default Database'}
label={'Default Authentication Database'}
type={'text'}
optional={true}
value={defaultDatabase ?? ''}
description={
'Default database will be the one you authenticate on. Leave this field empty if you want the default behaviour.'
'Authentication database used when authSource is not specified.'
}
/>
</FormFieldContainer>
Expand Down
35 changes: 5 additions & 30 deletions ...connect-form/src/components/advanced-options-tabs/advanced-tab/url-options-table.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ describe('UrlOptionsTable', function () {
});

it('renders view correctly', function () {
expect(screen.getByTestId('add-url-options-button')).to.exist;
expect(screen.getByTestId('url-options-table')).to.exist;
});

Expand All @@ -66,26 +65,7 @@ describe('UrlOptionsTable', function () {
});
});

it('renders new option entry with no value when user clicks on add new button', function () {
const button = screen.getByTestId('add-url-options-button');
fireEvent.click(button);

expect(screen.getByTestId('new-option-table-row')).to.exist;
expect(
screen.getByTestId('new-option-input-field').getAttribute('value')
).to.equal('');
});

it('renders error message when user clicks twice add new button', function () {
const button = screen.getByTestId('add-url-options-button');
fireEvent.click(button);
fireEvent.click(button);
expect(screen.findByText('Please complete the existing option.')).to
.exist;
});

it('renders selected key when user select a key', function () {
fireEvent.click(screen.getByTestId('add-url-options-button')); // Add new entry
fireEvent.click(screen.getByText(/select key/i)); // Click select button
fireEvent.click(screen.getByText(/appname/i)); // Select the option

Expand All @@ -109,7 +89,6 @@ describe('UrlOptionsTable', function () {
});

it('renders input value when user changes value', function () {
fireEvent.click(screen.getByTestId('add-url-options-button'));
fireEvent.change(screen.getByTestId('new-option-input-field'), {
target: { value: 'hello' },
});
Expand All @@ -123,7 +102,6 @@ describe('UrlOptionsTable', function () {
});

it('should update an option - when name changes', function () {
fireEvent.click(screen.getByTestId('add-url-options-button')); // Add new entry
fireEvent.click(screen.getByText(/select key/i)); // Click select button
fireEvent.click(screen.getByText(/appname/i)); // Select the option

Expand All @@ -140,6 +118,8 @@ describe('UrlOptionsTable', function () {
newKey: 'compressors',
value: '',
});

expect(screen.getByText(/select key/i)).to.exist; // renders an empty new option
});

it('should update an option - when value changes', function () {
Expand All @@ -156,13 +136,9 @@ describe('UrlOptionsTable', function () {
});
});

it('should delete a new option', function () {
fireEvent.click(screen.getByTestId('add-url-options-button'));
fireEvent.click(screen.getByTestId('new-option-delete-button'));
expect(() => {
screen.getByTestId('new-option-table-row');
}).to.throw;
expect(updateConnectionFormFieldSpy.callCount).to.equal(0);
it('should not render a delete button for a new option', function () {
expect(screen.getByTestId('new-option-table-row')).to.exist;
expect(screen.queryByTestId('new-option-delete-button')).to.not.exist;
});

// eslint-disable-next-line mocha/no-setup-in-describe
Expand Down Expand Up @@ -197,7 +173,6 @@ describe('UrlOptionsTable', function () {

it('renders view correctly', function () {
expect(screen.getByTestId('url-options-table')).to.exist;
expect(screen.getByTestId('add-url-options-button')).to.exist;
});

it('renders new option row and fields', function () {
Expand Down
135 changes: 57 additions & 78 deletions ...ages/connect-form/src/components/advanced-options-tabs/advanced-tab/url-options-table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,17 @@
import React, { ChangeEvent, useEffect, useCallback } from 'react';
import React, { ChangeEvent, useEffect } from 'react';
import {
spacing,
Table,
TableHeader,
Row,
Cell,
Button,
IconButton,
Icon,
TextInput,
Select,
Option,
OptionGroup,
Banner,
css,
spacing,
} from '@mongodb-js/compass-components';
import ConnectionStringUrl from 'mongodb-connection-string-url';
import type { MongoClientOptions } from 'mongodb';
Expand All @@ -22,23 +20,42 @@ import { editableUrlOptions, UrlOption } from '../../../utils/url-options';
import { UpdateConnectionFormField } from '../../../hooks/use-connect-form';

const optionSelectStyles = css({
width: 300,
width: spacing[5] * 9,
});

const optionNameCellStyles = css({
width: spacing[5] * 9,
});

const optionValueCellStyles = css({
// width: '30%',
});

const optionValueCellContentStyles = css({
display: 'flex',
flexDirection: 'row',
justifyContent: 'space-between',
justifyContent: 'flex-start',
alignItems: 'center',
width: '100%',
});

const addUrlOptionsButtonStyles = css({
textAlign: 'center',
marginTop: spacing[3],
marginBottom: spacing[2],
const valueInputStyles = css({
maxWidth: spacing[7],
});

const deleteOptionButtonStyle = css({
marginLeft: spacing[2],
});

function appendEmptyOption(
urlOptions: Partial<UrlOption>[]
): Partial<UrlOption>[] {
if (!urlOptions.length || urlOptions[urlOptions.length - 1].name) {
return [...urlOptions, { name: undefined, value: undefined }];
}

return urlOptions;
}

function getUrlOptions(connectionStringUrl: ConnectionStringUrl): UrlOption[] {
let opts: string[] = [];
const urlOptions: UrlOption[] = [];
Expand All @@ -65,39 +82,16 @@ function UrlOptionsTable({
connectionStringUrl: ConnectionStringUrl;
}): React.ReactElement {
const [options, setOptions] = React.useState<Partial<UrlOption>[]>([]);
const [errorMessage, setErrorMessage] = React.useState('');

// To fix UI issue that removes empty option(name -> undefined) when user
// removes an existing option (name -> defined) [because of the state update]
const [containsEmptyOption, setContainsEmptyOption] = React.useState(false);

useEffect(() => {
const options: Partial<UrlOption>[] = getUrlOptions(connectionStringUrl);
if (!options.length || containsEmptyOption) {
options.push({ name: undefined, value: undefined });
}
const options = appendEmptyOption(getUrlOptions(connectionStringUrl));
setOptions(options);
}, [connectionStringUrl, containsEmptyOption]);

const addUrlOption = useCallback(() => {
setErrorMessage('');
// Use case: User clicks on `Add url option` button and then clicked again without completing existing entry.
// Don't add another option in such case
if (options.find(({ name }) => !name)) {
setErrorMessage('Please complete the existing option.');
return;
}
const newOptions = [...options, { name: undefined, value: undefined }];
setOptions(newOptions);
setContainsEmptyOption(true);
}, [options]);
}, [connectionStringUrl]);

const updateUrlOption = (
currentName?: UrlOption['name'],
name?: UrlOption['name'],
value?: string
) => {
setErrorMessage('');
const indexOfUpdatedOption = options.findIndex(
(option) => option.name === currentName
);
Expand All @@ -107,7 +101,8 @@ function UrlOptionsTable({
};
const newOptions = [...options];
newOptions[indexOfUpdatedOption] = option;
setOptions(newOptions);

setOptions(appendEmptyOption(newOptions));

if (name) {
updateConnectionFormField({
Expand All @@ -122,24 +117,17 @@ function UrlOptionsTable({
};

const deleteUrlOption = (name?: UrlOption['name']) => {
setErrorMessage('');
if (!name) {
return;
}

const indexOfDeletedOption = options.findIndex(
(option) => option.name === name
);
const newOptions = [...options];
newOptions.splice(indexOfDeletedOption, 1);
if (newOptions.length === 0) {
newOptions.push({ name: undefined, value: undefined });
}
setOptions(newOptions);

if (!name) {
return;
}

if (newOptions.filter((x) => !x.name).length > 0) {
setContainsEmptyOption(true);
}
setOptions(appendEmptyOption(newOptions));

updateConnectionFormField({
type: 'delete-search-param',
Expand All @@ -164,7 +152,7 @@ function UrlOptionsTable({
datum.name ? `${datum.name}-table-row` : 'new-option-table-row'
}
>
<Cell>
<Cell className={optionNameCellStyles}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I asked this last time, but didn't get an answer: why are we using this table here at all? Seems like we are complicating layout both visually and from the code perspective, and the only thing this adds is the table headers that just duplicate the placeholders in the input fields

Copy link
Collaborator Author

@mcasimir mcasimir Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah is kinda tricky to align stuff, what you say makes sense to me. I don't know if this has to be a table or we could remove it and keep only the inputs. @Sgrinfy this is more a question for you.

I would keep it as is for now in this PR, and possibly change it in a follow-up one, there are a few other fixes to the new form that would be great to get in before.

<Select
className={optionSelectStyles}
placeholder="Select key"
Expand Down Expand Up @@ -202,8 +190,8 @@ function UrlOptionsTable({
))}
</Select>
</Cell>
<Cell>
<div className={optionValueCellStyles}>
<Cell className={optionValueCellStyles}>
<div className={optionValueCellContentStyles}>
<TextInput
onChange={(event: ChangeEvent<HTMLInputElement>) => {
event.preventDefault();
Expand All @@ -219,38 +207,29 @@ function UrlOptionsTable({
placeholder={'Value'}
aria-labelledby="Enter value"
value={datum.value}
className={valueInputStyles}
/>
<IconButton
data-testid={
datum.name
? `${datum.name}-delete-button`
: 'new-option-delete-button'
}
aria-label={`Delete option: ${datum.name ?? ''}`}
onClick={() => deleteUrlOption(datum.name)}
>
<Icon glyph="X" />
</IconButton>
{datum.name ? (
<IconButton
data-testid={
datum.name
? `${datum.name}-delete-button`
: 'new-option-delete-button'
}
aria-label={`Delete option: ${datum.name ?? ''}`}
onClick={() => deleteUrlOption(datum.name)}
className={deleteOptionButtonStyle}
>
<Icon glyph="X" />
</IconButton>
) : (
''
)}
</div>
</Cell>
</Row>
)}
</Table>
<div className={addUrlOptionsButtonStyles}>
<Button
data-testid="add-url-options-button"
onClick={() => addUrlOption()}
variant={'primaryOutline'}
size={'xsmall'}
>
Add url option
</Button>
</div>
{errorMessage && (
<Banner variant={'warning'} onClose={() => setErrorMessage('')}>
{errorMessage}
</Banner>
)}
</>
);
}
Expand Down