Skip to content
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.

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

POC of bifurcation of SimpleDropdown for FEMLab in PFE #6820

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

kieftrav
Copy link
Contributor

@kieftrav kieftrav commented Sep 11, 2023

Overview

Bifurcates the Dropdown in PFE & FEMLab interface to implement SimpleDropdown only in FEMLab.

Toward #6765

Required Manual Testing

PFE without FEMLab enabled for a Dropdown:

  • Should still display the "+ Add a Dropdown" interface
  • Should still have the "Allow Create" toggle in the Edit Modal

PFE with FEMLab enabled for a Dropdown should:

  • Should not display the "+ Add a Dropdown" interface
  • Should not have the "Allow Create" toggle in the Edit Modal

Review Checklist

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you npm ci and app works as expected?
  • If the component is in coffeescript, is it converted to ES6? Is it free of eslint errors? Is the conversion its own commit?
  • Are the tests passing locally and on GitHub Actions?

Optional

@coveralls
Copy link

coveralls commented Sep 11, 2023

Coverage Status

coverage: 57.185% (-0.009%) from 57.194% when pulling 4e8a1c7 on fem-simple-dropdown into ef3c417 on master.

@kieftrav
Copy link
Contributor Author

Example of PFE without FEMLab Enabled:

PFE-SimpleDropdown.mov

@kieftrav
Copy link
Contributor Author

Example of PFE with FEMLab Enabled for SimpleDropdown:

PFE-FEMLab-SimpleDropdown.mov

@kieftrav kieftrav force-pushed the fem-simple-dropdown branch 3 times, most recently from 0b11137 to 130f0b3 Compare September 14, 2023 13:46
@shaunanoordin shaunanoordin self-assigned this Sep 14, 2023
Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

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

PR Review

Affects: Project Builder -> Workflow Editor (PFE Lab & FEM Lab) -> Dropdown Tasks

This PR modifies the Dropdown Task in the Workflow Editor, making it so some dropdown task options are no longer available on FEM Lab mode. This is the first step in making a distinct & new version of dropdown tasks available on FEM Lab, while maintaining the original version of dropdown tasks on PFE Lab.

  • On PFE Lab (viewable by appending ?pfeLab=true to the URL), the controls marked in red in the screenshots below ARE visible and can be interacted with.
  • On FEM Lab (?femLab=true), the controls marked in red AREN'T visible.
image image

Testing

Tested with macOS + Chrome

Testing URLs:

Project 1966 has the 'dropdown' experimental tool enabled. Workflow 3773 has 1 task, which is a Dropdown Task with 5 options.

⚠️ tests will fail if Project 1966 has the 'shortcut' experimental tool enabled, due to the pfeLab variable not being passed properly in task-editor.jsx. This is a separate bug for a separate PR, though.

Testing steps:

  • Load the workflow. Observe the Dropdown Task.
    • PFE Lab version should show all controls (marked red in the screenshots above); FEM Lab version shouldn't show those controls.
  • Modify the Dropdown Task.
    • Change the question, change the dropdown's title, add & delete dropdown options.

❗ ISSUE DETECTED

Problem: on FEM Lab, if we attempt to change the dropdown's title OR check/uncheck the 'Required' box, an error will be thrown.

Screen.Recording.2023-09-15.at.01.59.27.mov

Analysis: this error occurs because in FEM Lab, the 'allowCreate' checkbox doesn't exist. However, the editSelect() function (triggered by title's/required's/allowCreate's onChange handler) still looks for @refs.allowCreate.checked.

Solution: check for @refs.allowCreate before using @refs.allowCreate.checked

Status

This PR requires 3x changes before it's good to merge.

Requested changes are all for app/classifier/tasks/dropdown/dropdown-dialog.cjsx

Code cleanup:

  • remove isFemLab: false in getDefaultProps() OR replace it with pfeLab: false
  • remove stray {props.isFemLab}

Reason: isFemLab is no longer used.

Code fixes:

  • in editSelect() (the function, not the confusingly similarly named state), change select.allowCreate = @refs.allowCreate.checked to select.allowCreate = if @refs.allowCreate then @refs.allowCreate.checked else false

Or, in full:

  editSelect: ->
    select = @state.editSelect
    select.title = @refs.title.value
    select.required = @refs.required.checked
    select.allowCreate = if @refs.allowCreate then @refs.allowCreate.checked else false
    @setState editSelect: select

Reason: fixes the issue noted above. When in FEM Lab mode, the allowCreate value must always be false.

Other than these 3 CRs, the code changes look good, functionality checks out, and the updated test properly checks for the PFE & FEM differences. Sweet. 👌

app/classifier/tasks/dropdown/dropdown-dialog.cjsx Outdated Show resolved Hide resolved
app/classifier/tasks/dropdown/dropdown-dialog.cjsx Outdated Show resolved Hide resolved
@goplayoutside3
Copy link
Contributor

Just a quick note I'm editing the description to link Issue #6765 to this PR.

@shaunanoordin shaunanoordin self-requested a review September 20, 2023 11:10
Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

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

PR Review (Update)

Awesome, thank you for making those changes! This PR is now LGTM 👍

@kieftrav kieftrav merged commit 0e3ca29 into master Sep 20, 2023
@kieftrav kieftrav deleted the fem-simple-dropdown branch September 20, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants