From b531cdc3b95ab72019c5880332e36af509730270 Mon Sep 17 00:00:00 2001
From: Katerina Koukiou <kkoukiou@redhat.com>
Date: Thu, 25 Mar 2021 12:58:20 +0100
Subject: [PATCH] Use selectable DataList for the change remote modal dialog's
 list

Also put the content of the 'Add new repository' modal into another
dialog. The previous dialog had too many forms inside.

Fixes #176
---
 src/app.jsx               |   2 +-
 src/changeRemoteModal.jsx | 213 +++++++++++++++++++-------------------
 src/ostree.scss           |  21 +---
 test/check-ostree         |  16 +--
 4 files changed, 120 insertions(+), 132 deletions(-)

diff --git a/src/app.jsx b/src/app.jsx
index 271397e1..526f8c67 100644
--- a/src/app.jsx
+++ b/src/app.jsx
@@ -28,7 +28,7 @@ import './lib/patternfly/patternfly-4-cockpit.scss';
 import {
     Title, Button, Alert,
     EmptyState, EmptyStateVariant, EmptyStateIcon, EmptyStateBody,
-    DataList, DataListItem, DataListItemRow, DataListItemCells, DataListCell, DataListContent, /* DataListAction, */
+    DataList, DataListItem, DataListItemRow, DataListItemCells, DataListCell, DataListContent,
     Page, PageSection, PageSectionVariants,
     Select, SelectOption,
     Spinner,
diff --git a/src/changeRemoteModal.jsx b/src/changeRemoteModal.jsx
index effb65d6..246cb27e 100644
--- a/src/changeRemoteModal.jsx
+++ b/src/changeRemoteModal.jsx
@@ -23,11 +23,15 @@ import React, { useState } from 'react';
 import PropTypes from "prop-types";
 
 import {
-    Title, Button, Alert, AlertActionCloseButton,
+    Button, Alert, AlertActionCloseButton,
     Form, FormGroup, ActionGroup,
     TextInput, TextArea,
     Checkbox,
-    SimpleList, SimpleListItem,
+    DataList,
+    DataListCell,
+    DataListItem,
+    DataListItemRow,
+    DataListItemCells,
     Modal,
 } from '@patternfly/react-core';
 import { PencilAltIcon, AddCircleOIcon } from '@patternfly/react-icons';
@@ -59,66 +63,60 @@ export const ChangeRemoteModal = ({ setIsModalOpen, isModalOpen, remotesList, cu
     </>;
 
     return (
-        <Modal title={_("Change Repository")}
-               position="top"
-               variant="medium"
-               appendTo={document.body}
-               isOpen={isModalOpen}
-               onClose={() => setIsModalOpen(false)}
-               footer={footer}>
-            <>
-                {error && <Alert variant="danger" isInline title={error} />}
-                <SimpleList className="remote-select" onSelect={(_, currentItemProps) => setSelectedRemote(currentItemProps.id)}>
-                    {(remotesList || []).map(remote => {
-                        return (
-                            (!editRepoDialogOpen || editRepoDialogOpen.name !== remote)
-                            ? <SimpleListItem key={remote}
-                                              id={remote}
-                                              component="a"
-                                              onClick={ev => {
-                                                  ev.stopPropagation();
-                                                  ev.preventDefault();
-                                              }}
-                                              isCurrent={remote === selectedRemote}>
-                                <span>{remote}</span>
-                                <Button onClick={ ev => {
-                                            remotes.loadRemoteSettings(remote)
-                                                .then(remoteSettings => setEditRepoDialogOpen(Object.assign(remoteSettings, { name: remote })));
-                                        }}
-                                        className="edit-remote"
-                                        variant="secondary">
-                                    <PencilAltIcon />
-                                </Button>
-                            </SimpleListItem>
-                            : <div key={remote} className="pf-c-simple-list__item-link">
-                                <EditRemoteForm setEditRepoDialogOpen={setEditRepoDialogOpen} remoteSettings={editRepoDialogOpen} refreshRemotes={refreshRemotes} />
-                            </div>
-                        );
-                    }).concat([
-                        !addNewRepoDialogOpen
-                        ? <SimpleListItem component="a"
-                                        onClick={ev => {
-                                            ev.stopPropagation();
-                                            ev.preventDefault();
-                                        }}
-                                        key="add-new">
-                            <Button onClick={ev => {
-                                ev.stopPropagation();
-                                ev.preventDefault();
-                                setAddNewRepoDialogOpen(true);
-                            }}
-                                   variant="link"
-                                   icon={<AddCircleOIcon />}
-                                   id="add-new-remote-btn"
-                                   iconPosition="left">{_("Add New Repository")}</Button>
-                        </SimpleListItem>
-                        : <div key="add new" className="pf-c-simple-list__item-link">
-                            <AddNewRepoForm refreshRemotes={refreshRemotes} setAddNewRepoDialogOpen={setAddNewRepoDialogOpen} />
-                        </div>
-                    ])}
-                </SimpleList>
-            </>
-        </Modal>
+        <>
+            <Modal title={_("Change Repository")}
+                   position="top"
+                   variant="medium"
+                   appendTo={document.body}
+                   isOpen={isModalOpen && !addNewRepoDialogOpen}
+                   onClose={() => setIsModalOpen(false)}
+                   footer={footer}>
+                <>
+                    {error && <Alert variant="danger" isInline title={error} />}
+                    <DataList isCompact aria-label={_("Remote select")}
+                              selectedDataListItemId={selectedRemote}
+                              onSelectDataListItem={setSelectedRemote}
+                              className="remote-select">
+                        {(remotesList || []).map(remote => {
+                            return (
+                                <DataListItem id={remote} key={remote}>
+                                    <DataListItemRow>
+                                        <DataListItemCells
+                                            dataListCells={[
+                                                <DataListCell key="remote-name">
+                                                    {remote}
+                                                </DataListCell>,
+                                                (!editRepoDialogOpen || editRepoDialogOpen.name !== remote)
+                                                ? <DataListCell key="remote-edit" alignRight isFilled={false}>
+                                                    <Button onClick={ ev => {
+                                                                remotes.loadRemoteSettings(remote)
+                                                                    .then(remoteSettings => setEditRepoDialogOpen(Object.assign(remoteSettings, { name: remote })));
+                                                            }}
+                                                            isSmall
+                                                            className="edit-remote"
+                                                            variant="secondary">
+                                                        <PencilAltIcon />
+                                                    </Button>
+                                                </DataListCell>
+                                                : <DataListCell key="edit-repo-form" width={4}>
+                                                    <EditRemoteForm setEditRepoDialogOpen={setEditRepoDialogOpen} remoteSettings={editRepoDialogOpen} refreshRemotes={refreshRemotes} />
+                                                </DataListCell>
+                                            ]}
+                                        />
+                                    </DataListItemRow>
+                                </DataListItem>
+                            );
+                        })}
+                    </DataList>
+                    <Button onClick={() => setAddNewRepoDialogOpen(true)}
+                         variant="link"
+                         icon={<AddCircleOIcon />}
+                         id="add-new-remote-btn"
+                         iconPosition="left">{_("Add New Repository")}</Button>
+                </>
+            </Modal>
+            {addNewRepoDialogOpen && <AddNewRepoForm refreshRemotes={refreshRemotes} setAddNewRepoDialogOpen={setAddNewRepoDialogOpen} />}
+        </>
     );
 };
 
@@ -151,46 +149,52 @@ const AddNewRepoForm = ({ setAddNewRepoDialogOpen, refreshRemotes }) => {
     };
 
     return (
-        <Form isHorizontal>
-            <Title headingLevel="h3" size="l">
-                {_("Add New Repository")}
-            </Title>
-            {addNewRepoError && <Alert variant="danger" isInline title={addNewRepoError} />}
-            <FormGroup label={_("Name")}
-                       fieldId="new-remote-name"
-                       helperTextInvalid={_("Please provide a valid name")}
-                       validated={(hasValidation && !newRepoName.trim().length) ? "error" : undefined}
-                       isRequired>
-                <TextInput id="new-remote-name"
-                           value={newRepoName}
-                           isRequired
-                           type="text"
-                           onChange={name => setNewRepoName(name)} />
-            </FormGroup>
-            <FormGroup label={_("URL")}
-                       fieldId="new-remote-url"
-                       helperTextInvalid={_("Please provide a valid URL")}
-                       validated={(hasValidation && !newRepoURL.trim().length) ? "error" : undefined}
-                       isRequired>
-                <TextInput id="new-remote-url"
-                           value={newRepoURL}
-                           isRequired
-                           type="text"
-                           onChange={url => setNewRepoURL(url)} />
-            </FormGroup>
-            <FormGroup fieldId="new-gpg-verify">
-                <Checkbox label={_("Use trusted GPG key")}
-                          id="new-gpg-verify"
-                          isChecked={newRepoTrusted}
-                          onChange={(checked, ev) => {
-                              setNewRepoTrusted(checked);
-                          }} />
-            </FormGroup>
-            <ActionGroup>
-                <Button id="add-remote-btn" onClick={() => onAddRemote()} variant="primary">{_("Add")}</Button>
-                <Button onClick={() => setAddNewRepoDialogOpen(false)} variant="link">{_("Cancel")}</Button>
-            </ActionGroup>
-        </Form>
+        <Modal title={_("Add New Repository")}
+               position="top"
+               variant="small"
+               appendTo={document.body}
+               isOpen
+               onClose={() => setAddNewRepoDialogOpen(false)}
+               footer={
+                   <>
+                       <Button id="add-remote-btn" onClick={() => onAddRemote()} variant="primary">{_("Add")}</Button>
+                       <Button onClick={() => setAddNewRepoDialogOpen(false)} variant="link">{_("Cancel")}</Button>
+                   </>
+               }>
+            <Form isHorizontal>
+                {addNewRepoError && <Alert variant="danger" isInline title={addNewRepoError} />}
+                <FormGroup label={_("Name")}
+                           fieldId="new-remote-name"
+                           helperTextInvalid={_("Please provide a valid name")}
+                           validated={(hasValidation && !newRepoName.trim().length) ? "error" : undefined}
+                           isRequired>
+                    <TextInput id="new-remote-name"
+                               value={newRepoName}
+                               isRequired
+                               type="text"
+                               onChange={name => setNewRepoName(name)} />
+                </FormGroup>
+                <FormGroup label={_("URL")}
+                           fieldId="new-remote-url"
+                           helperTextInvalid={_("Please provide a valid URL")}
+                           validated={(hasValidation && !newRepoURL.trim().length) ? "error" : undefined}
+                           isRequired>
+                    <TextInput id="new-remote-url"
+                               value={newRepoURL}
+                               isRequired
+                               type="text"
+                               onChange={url => setNewRepoURL(url)} />
+                </FormGroup>
+                <FormGroup fieldId="new-gpg-verify">
+                    <Checkbox label={_("Use trusted GPG key")}
+                              id="new-gpg-verify"
+                              isChecked={newRepoTrusted}
+                              onChange={(checked, ev) => {
+                                  setNewRepoTrusted(checked);
+                              }} />
+                </FormGroup>
+            </Form>
+        </Modal>
     );
 };
 AddNewRepoForm.propTypes = {
@@ -219,13 +223,10 @@ const EditRemoteForm = ({ remoteSettings, setEditRepoDialogOpen, refreshRemotes
     };
 
     return (
-        <Form isHorizontal>
+        <Form>
             {error && <Alert variant="danger" isInline
                              action={<AlertActionCloseButton onClose={() => this.setState({ error: undefined })} />}
                              title={error} />}
-            <Title headingLevel="h3" size="l">
-                {remoteSettings.name}
-            </Title>
             <FormGroup label={_("URL")}
                 fieldId="edit-remote-url">
                 <TextInput id="edit-remote-url"
diff --git a/src/ostree.scss b/src/ostree.scss
index 5309fa87..83342e7e 100644
--- a/src/ostree.scss
+++ b/src/ostree.scss
@@ -54,23 +54,6 @@
     max-height: calc(100vh - var(--pf-global--spacer--2xl));
 }
 
-// Create space between the remote name and the edit action
-.remote-select .pf-c-simple-list__item-link {
-    display: flex;
-    justify-content: space-between;
-}
-
-.simple-list-form-header {
-    display: flex;
-    justify-content: space-between;
-    flex-flow: wrap;
-}
-
-.simple-list-form-actions > .pf-c-button {
-    margin-left: .3em;
-    margin-right: .3em;
-}
-
 .pf-c-form.pf-m-horizontal {
     width: 100%;
 }
@@ -108,3 +91,7 @@
 .deployment-error svg {
     margin-right: .5rem;
 }
+
+.remote-select {
+    padding-bottom: var(--pf-global--spacer--md);
+}
diff --git a/test/check-ostree b/test/check-ostree
index a1bd6303..237af846 100755
--- a/test/check-ostree
+++ b/test/check-ostree
@@ -446,7 +446,7 @@ class OstreeCase(MachineCase):
         b.click("#change-repo")
 
         b.wait_in_text(".pf-c-modal-box h1", "Change Repository")
-        b.wait_in_text(".pf-c-modal-box .pf-c-simple-list .pf-m-current", "local")
+        b.wait_in_text(".pf-c-modal-box .remote-select .pf-m-selected", "local")
         b.wait_in_text(".pf-c-modal-box li:last", "Add New Repository")
 
         b.click(".pf-c-modal-box li:last button")
@@ -454,7 +454,7 @@ class OstreeCase(MachineCase):
         b.set_input_text(".pf-c-modal-box #new-remote-url", "http://localhost:12344")
         b.set_input_text(".pf-c-modal-box #new-remote-name", "zremote test")
         b.click(".pf-c-modal-box #add-remote-btn")
-        b.wait_in_text(".pf-c-modal-box .pf-c-simple-list__item-link .pf-m-danger", "Invalid remote name")
+        b.wait_in_text(".pf-c-modal-box .pf-m-danger", "Invalid remote name")
         b.set_input_text(".pf-c-modal-box #new-remote-name", "zremote-test1")
         b.click(".pf-c-modal-box #add-remote-btn")
 
@@ -462,7 +462,7 @@ class OstreeCase(MachineCase):
         b.wait_not_present(".pf-c-modal-box #add-remote-btn")
         b.wait_not_present(".pf-c-modal-box .pf-c-modal-box__footer button:disabled")
         b.wait_visible(".pf-c-modal-box .pf-c-modal-box__footer button.pf-m-primary")
-        b.wait_in_text(".pf-c-modal-box .pf-c-simple-list .pf-m-current", "local")
+        b.wait_in_text(".pf-c-modal-box .remote-select .pf-m-selected", "local")
         b.wait_in_text(".pf-c-modal-box li:last button", "Add New Repository")
         b.click(".pf-c-modal-box #zremote-test1 a")
 
@@ -591,7 +591,7 @@ class OstreeCase(MachineCase):
         # Switching to branch shows pulled
         b.wait_text("#change-repo", "local")
         b.click("#change-repo")
-        b.wait_visible(".pf-c-modal-box .pf-c-simple-list")
+        b.wait_visible(".pf-c-modal-box .remote-select")
         b.click(".pf-c-modal-box #zremote-test1 a")
         b.click(".pf-c-modal-box .pf-c-modal-box__footer button.pf-m-primary")
         b.wait_not_present(".pf-c-modal-box")
@@ -614,12 +614,12 @@ class OstreeCase(MachineCase):
         b.click(".pf-c-modal-box button.pf-m-danger")
         b.wait_not_present(".pf-c-modal-box .pf-c-form")
 
-        b.wait_not_in_text(".pf-c-modal-box .pf-c-simple-list", "zremote-test1")
-        b.wait_in_text(".pf-c-modal-box .pf-c-simple-list", "local")
-        b.wait_not_present(".pf-c-modal-box .pf-c-simple-list .pf-m-current")
+        b.wait_not_in_text(".pf-c-modal-box .remote-select", "zremote-test1")
+        b.wait_in_text(".pf-c-modal-box .remote-select", "local")
+        b.wait_not_present(".pf-c-modal-box .remote-select .pf-m-selected")
         b.wait_visible(".pf-c-modal-box .pf-c-modal-box__footer button:disabled")
         b.click(".pf-c-modal-box #local a")
-        b.wait_visible(".pf-c-modal-box .pf-c-simple-list .pf-m-current")
+        b.wait_visible(".pf-c-modal-box .remote-select .pf-m-selected")
         b.click(".pf-c-modal-box .pf-c-modal-box__footer button.pf-m-primary")
         b.wait_not_present(".pf-c-modal-box")
         b.wait_text("#change-repo", "local")