Merge pull request #1614 from emilyanndavis/912-workbench-form-valida…

…tion-ui-updates Workbench Form Validation UI Updates
natcap · Aug 20, 2024 · f18b211 · f18b211
2 parents e819660 + 69d33b2
commit f18b211
Show file tree

Hide file tree

Showing 9 changed files with 127 additions and 84 deletions.
diff --git a/HISTORY.rst b/HISTORY.rst
@@ -35,10 +35,10 @@
 
 .. :changelog:
 
-
-..
-  Unreleased Changes
-  ------------------
+Unreleased Changes
+------------------
+* Workbench
+  * Several small updates to the model input form UI to improve usability and visual consistency (https://github.com/natcap/invest/issues/912)
 
 3.14.2 (2024-05-29)
 -------------------

diff --git a/src/natcap/invest/validation.py b/src/natcap/invest/validation.py
@@ -65,7 +65,9 @@
     'BBOX_NOT_INTERSECT': gettext(
         'Not all of the spatial layers overlap each '
         'other. All bounding boxes must intersect: {bboxes}'),
-    'NEED_PERMISSION': gettext(
+    'NEED_PERMISSION_DIRECTORY': gettext(
+        'You must have {permission} access to this directory'),
+    'NEED_PERMISSION_FILE': gettext(
         'You must have {permission} access to this file'),
     'WRONG_GEOM_TYPE': gettext('Geometry type must be one of {allowed}')
 }
@@ -191,7 +193,7 @@ def check_directory(dirpath, must_exist=True, permissions='rx', **kwargs):
                 dirpath = parent
                 break
 
-    permissions_warning = check_permissions(dirpath, permissions)
+    permissions_warning = check_permissions(dirpath, permissions, True)
     if permissions_warning:
         return permissions_warning
 
@@ -218,7 +220,7 @@ def check_file(filepath, permissions='r', **kwargs):
         return permissions_warning
 
 
-def check_permissions(path, permissions):
+def check_permissions(path, permissions, is_directory=False):
     """Validate permissions on a filesystem object.
 
     This function uses ``os.access`` to determine permissions access.
@@ -229,6 +231,8 @@ def check_permissions(path, permissions):
             and/or ``x`` (lowercase), indicating read, write, and execute
             permissions (respectively) that the filesystem object at ``path``
             must have.
+        is_directory (boolean): Indicates whether the path refers to a directory
+            (True) or a file (False). Defaults to False.
 
     Returns:
         A string error message if an error was found.  ``None`` otherwise.
@@ -239,7 +243,8 @@ def check_permissions(path, permissions):
             ('w', os.W_OK, 'write'),
             ('x', os.X_OK, 'execute')):
         if letter in permissions and not os.access(path, mode):
-            return MESSAGES['NEED_PERMISSION'].format(permission=letter)
+            message_key = 'NEED_PERMISSION_DIRECTORY' if is_directory else 'NEED_PERMISSION_FILE'
+            return MESSAGES[message_key].format(permission=descriptor)
 
 
 def _check_projection(srs, projected, projection_units):

diff --git a/workbench/src/renderer/components/SetupTab/ArgInput/index.jsx b/workbench/src/renderer/components/SetupTab/ArgInput/index.jsx
@@ -41,38 +41,44 @@ function filterSpatialOverlapFeedback(message, filepath) {
 
 function FormLabel(props) {
   const {
-    argkey, argname, required, units,
+    argkey, argname, argtype, required, units,
   } = props;
 
+  const userFriendlyArgType = parseArgType(argtype);
+  const optional = typeof required === 'boolean' && !required;
+  const includeComma = userFriendlyArgType && optional;
+
   return (
     <Form.Label column sm="3" htmlFor={argkey}>
-      <span id="argname">
-        {argname}
-      </span>
-      <span>
-        {
-          (typeof required === 'boolean' && !required)
-            ? <em> ({i18n.t('optional')})</em>
-            : <React.Fragment />
-        }
-        {/* display units at the end of the arg name, if applicable */}
-        { (units && units !== 'unitless') ? ` (${units})` : <React.Fragment /> }
-      </span>
+      <span className="argname">{argname} </span>
+      {
+       (userFriendlyArgType || optional) && 
+        <span>
+          (
+            {userFriendlyArgType}
+            {includeComma && ', '}
+            {optional && <em>{i18n.t('optional')}</em>}
+          )
+        </span>
+      }
+      {/* display units at the end of the arg name, if applicable */}
+      { (units && units !== 'unitless') ? <span> ({units})</span> : <React.Fragment /> }
     </Form.Label>
   );
 }
 
 FormLabel.propTypes = {
   argkey: PropTypes.string.isRequired,
   argname: PropTypes.string.isRequired,
+  argtype: PropTypes.string.isRequired,
   required: PropTypes.oneOfType(
     [PropTypes.string, PropTypes.bool]
   ),
   units: PropTypes.string,
 };
 
 function Feedback(props) {
-  const { argkey, argtype, message } = props;
+  const { argkey, message } = props;
   return (
     // d-block class is needed because of a bootstrap bug
     // https://github.com/twbs/bootstrap/issues/29439
@@ -81,13 +87,12 @@ function Feedback(props) {
       type="invalid"
       id={`${argkey}-feedback`}
     >
-      {`${i18n.t(argtype)} : ${(message)}`}
+      {message}
     </Form.Control.Feedback>
   );
 }
 Feedback.propTypes = {
   argkey: PropTypes.string.isRequired,
-  argtype: PropTypes.string.isRequired,
   message: PropTypes.string,
 };
 Feedback.defaultProps = {
@@ -127,6 +132,30 @@ function dragLeavingHandler(event) {
   event.currentTarget.classList.remove('input-dragging');
 }
 
+function parseArgType(argtype) {
+  const { t, i18n } = useTranslation();
+  // These types benefit from more descriptive placeholder text.
+  let userFriendlyArgType;
+  switch (argtype) {
+    case 'freestyle_string':
+      userFriendlyArgType = t('text');
+      break;
+    case 'percent':
+      userFriendlyArgType = t('percent: a number from 0 - 100');
+      break;
+    case 'ratio':
+      userFriendlyArgType = t('ratio: a decimal from 0 - 1');
+      break;
+    case 'boolean':
+    case 'option_string':
+      userFriendlyArgType = '';
+      break;
+    default:
+      userFriendlyArgType = t(argtype);
+  }
+  return userFriendlyArgType;
+}
+
 export default function ArgInput(props) {
   const inputRef = useRef();
 
@@ -147,8 +176,6 @@ export default function ArgInput(props) {
   } = props;
   let { validationMessage } = props;
 
-  const { t, i18n } = useTranslation();
-
   // Occasionaly we want to force a scroll to the end of input fields
   // so that the most important part of a filepath is visible.
   // scrollEventCount changes on drop events and on use of the browse button.
@@ -207,20 +234,7 @@ export default function ArgInput(props) {
   }
 
   // These types benefit from more descriptive placeholder text.
-  let placeholderText;
-  switch (argSpec.type) {
-    case 'freestyle_string':
-      placeholderText = t('text');
-      break;
-    case 'percent':
-      placeholderText = t('percent: a number from 0 - 100');
-      break;
-    case 'ratio':
-      placeholderText = t('ratio: a decimal from 0 - 1');
-      break;
-    default:
-      placeholderText = t(argSpec.type);
-  }
+  const placeholderText = parseArgType(argSpec.type);
 
   let form;
   if (argSpec.type === 'boolean') {
@@ -246,6 +260,8 @@ export default function ArgInput(props) {
         onChange={handleChange}
         onFocus={handleChange}
         disabled={!enabled}
+        isValid={enabled && isValid}
+        custom
       >
         {
           Array.isArray(dropdownOptions) ?
@@ -278,6 +294,7 @@ export default function ArgInput(props) {
           onDragOver={dragOverHandler}
           onDragEnter={dragEnterHandler}
           onDragLeave={dragLeavingHandler}
+          aria-describedby={`${argkey}-feedback`}
         />
         {fileSelector}
       </React.Fragment>
@@ -294,6 +311,7 @@ export default function ArgInput(props) {
       <FormLabel
         argkey={argkey}
         argname={argSpec.name}
+        argtype={argSpec.type}
         required={argSpec.required}
         units={argSpec.units} // undefined for all types except number
       />

diff --git a/workbench/src/renderer/styles/style.css b/workbench/src/renderer/styles/style.css
@@ -483,23 +483,33 @@ exceed 100% of window.*/
 }
 
 .args-form .form-group {
-  align-items: center;
+  align-items: flex-start;
 }
 
-#argname {
+.argname {
   text-transform: capitalize;
   font-weight: 600;
 }
 
-.args-form .form-control {
+.args-form .form-control,
+.custom-select {
   font-family: monospace;
-  font-size:1.3rem;
+  font-size: 1.3rem;
+}
+
+.args-form {
+  --form-field-padding-right: 2em;
+}
+
+.args-form .form-label {
+  padding-top: 0;
+  padding-bottom: 0;
 }
 
 .args-form .form-control[type=text] {
   /*always hold space for a validation mark so the rightmost
   text is never hidden by the mark when it appears.*/
-  padding-right: 2em;
+  padding-right: var(--form-field-padding-right);
 }
 
 input.input-dragging {
@@ -522,14 +532,24 @@ input[type=text]::placeholder {
   font-size: 0.9rem;
   font-family: monospace;
   white-space: pre-wrap;
-  padding-left: 3rem;
-  text-indent: -3rem;
+  padding-left: calc(3.5rem + 2px);
 }
 
 .args-form svg {
   font-size: 1.5rem;
 }
 
+.custom-select,
+.custom-select.is-valid {
+  --caret-width: 1.875rem;
+  padding-right: var(--form-field-padding-right);
+  /* Custom dropdown icon is react-icons/md/MdKeyboardArrowDown, as a URL-encoded SVG */
+  background-image: url('data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg" stroke="currentColor" fill="currentColor" stroke-width="0" viewBox="0 0 24 24" height="200px" width="200px"%3E%3Cpath fill="none" d="M0 0h24v24H0V0z"%3E%3C/path%3E%3Cpath d="M7.41 8.59 12 13.17l4.59-4.58L18 10l-6 6-6-6 1.41-1.41z"%3E%3C/path%3E%3C/svg%3E');
+  background-repeat: no-repeat;
+  background-position: center right calc((var(--form-field-padding-right) - var(--caret-width)) / 2);
+  background-size: var(--caret-width);
+}
+
 /* InVEST Log Tab */
 #log-display {
   overflow: auto;

diff --git a/workbench/tests/binary_tests/puppet.test.js b/workbench/tests/binary_tests/puppet.test.js
@@ -183,16 +183,16 @@ test('Run a real invest model', async () => {
   const argsForm = await page.waitForSelector('.args-form');
   const typeDelay = 10;
   const workspace = await argsForm.waitForSelector(
-    'aria/[name="Workspace"][role="textbox"]');
+    'aria/[name="Workspace (directory)"][role="textbox"]');
   await workspace.type(TMP_DIR, { delay: typeDelay });
   const aoi = await argsForm.waitForSelector(
-    'aria/[name="Area Of Interest"][role="textbox"]');
+    'aria/[name="Area Of Interest (vector)"][role="textbox"]');
   await aoi.type(TMP_AOI_PATH, { delay: typeDelay });
   const startYear = await argsForm.waitForSelector(
-    'aria/[name="Start Year"][role="textbox"]');
+    'aria/[name="Start Year (number)"][role="textbox"]');
   await startYear.type('2008', { delay: typeDelay });
   const endYear = await argsForm.waitForSelector(
-    'aria/[name="End Year"][role="textbox"]');
+    'aria/[name="End Year (number)"][role="textbox"]');
   await endYear.type('2012', { delay: typeDelay });
   await page.screenshot({ path: `${SCREENSHOT_PREFIX}4-complete-setup-form.png` });
 

diff --git a/workbench/tests/renderer/app.test.js b/workbench/tests/renderer/app.test.js
@@ -122,7 +122,7 @@ describe('Various ways to open and close InVEST models', () => {
     expect(setupTab.classList.contains('active')).toBeTruthy();
 
     // Expect some arg values that were loaded from the saved job:
-    const input = await findByLabelText(SAMPLE_SPEC.args.workspace_dir.name);
+    const input = await findByLabelText((content) => content.startsWith(SAMPLE_SPEC.args.workspace_dir.name));
     expect(input).toHaveValue(
       argsValues.workspace_dir
     );
@@ -155,7 +155,7 @@ describe('Various ways to open and close InVEST models', () => {
     expect(executeButton).toBeDisabled();
     const setupTab = await findByText('Setup');
     const input = await findByLabelText(
-      SAMPLE_SPEC.args.carbon_pools_path.name
+      (content) => content.startsWith(SAMPLE_SPEC.args.carbon_pools_path.name)
     );
     expect(setupTab.classList.contains('active')).toBeTruthy();
     expect(input).toHaveValue(mockDatastack.args.carbon_pools_path);

diff --git a/workbench/tests/renderer/invest_subprocess.test.js b/workbench/tests/renderer/invest_subprocess.test.js
@@ -145,7 +145,7 @@ describe('InVEST subprocess testing', () => {
     );
     await userEvent.click(carbon);
     const workspaceInput = await findByLabelText(
-      `${spec.args.workspace_dir.name}`
+      (content) => content.startsWith(spec.args.workspace_dir.name)
     );
     await userEvent.type(workspaceInput, fakeWorkspace);
     const execute = await findByRole('button', { name: /Run/ });
@@ -195,7 +195,7 @@ describe('InVEST subprocess testing', () => {
     );
     await userEvent.click(carbon);
     const workspaceInput = await findByLabelText(
-      `${spec.args.workspace_dir.name}`
+      (content) => content.startsWith(spec.args.workspace_dir.name)
     );
     await userEvent.type(workspaceInput, fakeWorkspace);
 
@@ -250,7 +250,7 @@ describe('InVEST subprocess testing', () => {
     );
     await userEvent.click(carbon);
     const workspaceInput = await findByLabelText(
-      `${spec.args.workspace_dir.name}`
+      (content) => content.startsWith(spec.args.workspace_dir.name)
     );
     await userEvent.type(workspaceInput, fakeWorkspace);
 
@@ -301,7 +301,7 @@ describe('InVEST subprocess testing', () => {
     );
     await userEvent.click(carbon);
     const workspaceInput = await findByLabelText(
-      `${spec.args.workspace_dir.name}`
+      (content) => content.startsWith(spec.args.workspace_dir.name)
     );
     await userEvent.type(workspaceInput, fakeWorkspace);
 

diff --git a/workbench/tests/renderer/investtab.test.js b/workbench/tests/renderer/investtab.test.js
@@ -299,9 +299,9 @@ describe('Sidebar Buttons', () => {
     const setupTab = await findByRole('tab', { name: 'Setup' });
     expect(setupTab.classList.contains('active')).toBeTruthy();
 
-    const input1 = await findByLabelText(spec.args.workspace.name);
+    const input1 = await findByLabelText((content) => content.startsWith(spec.args.workspace.name));
     expect(input1).toHaveValue(mockDatastack.args.workspace);
-    const input2 = await findByLabelText(spec.args.port.name);
+    const input2 = await findByLabelText((content) => content.startsWith(spec.args.port.name));
     expect(input2).toHaveValue(mockDatastack.args.port);
   });
 
@@ -427,8 +427,8 @@ describe('InVEST Run Button', () => {
     const runButton = await findByRole('button', { name: /Run/ });
     expect(runButton).toBeDisabled();
 
-    const a = await findByLabelText(`${spec.args.a.name}`);
-    const b = await findByLabelText(`${spec.args.b.name}`);
+    const a = await findByLabelText((content) => content.startsWith(spec.args.a.name));
+    const b = await findByLabelText((content) => content.startsWith(spec.args.b.name));
 
     expect(a).toHaveClass('is-invalid');
     expect(b).toHaveClass('is-invalid');