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

Limit FEMLab Dropdown to only allow between 4-200 options. #6872

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

kieftrav
Copy link
Contributor

@kieftrav kieftrav commented Oct 10, 2023

Staging branch URL: https://pr-6872.pfe-preview.zooniverse.org

Overview

Limit the SimpleDropdown within FEMLab to between 4-200 options.

Toward #6765

PFE without FEMLab enabled for a Dropdown:

  • Should not limit options to greater than 4
  • Should not limit options to less than 200

PFE with FEMLab enabled for a Dropdown should:

  • Should limit options to greater than 4
  • Should limit options to less than 200

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?

@coveralls
Copy link

coveralls commented Oct 10, 2023

Coverage Status

coverage: 57.173% (-0.03%) from 57.201% when pulling 668a3ae on simple-dropdown-limit into ec6bb04 on master.

@kieftrav
Copy link
Contributor Author

The notification and enforcement of the limitation for FEMLab shows a standard window alert because that is what the current code validation does for a missing dropdown title. My preference is to follow the existing code style of using the window browser alert to create a consistent UI experience (acknowledging we all recognize it as sub-optimal). Attached are screenshots of the UI as discussed with @seanmiller26 and @shaunanoordin. If we'd like the UI to diverge from the pre-existing pattern we can do that with a relatively small amount of coding.

Test-display-4-option-limit Existing-missing-title-alert Less-than-4-alert Greater-than-200-alert

@seanmiller26
Copy link

Considering that we want a consistent UI experience, this LGTM.

@eatyourgreens
Copy link
Contributor

There’s an Alert component, which probably can be styled, but it’s probably less accessible than window.alert.
https://github.com/zooniverse/Panoptes-Front-End/blob/ebfa8b33ef6ed8faa3ecf621c1d6ea5e8d3449d1/app/lib/alert.jsx

@shaunanoordin shaunanoordin self-assigned this Oct 12, 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: Dropdown Tasks editor (FEM Lab only)

This PR modifies the Dropdown Tasks editor, in the Project Builder (FEM Lab ver), so that it only accepts dropdown lists with a minimum of 4 and a maximum of 200 options.

  • When the dropdown list modal is open, it will reject any attempt to Save changes if the number of options doesn't meet the criteria.
    • In the screenshot below, after adding 3 items to the list, a window.alert appears when clicking 'Save', preventing the list of 3 items from being saved. As a project owner, I either need to add at least 1 more item, or press 'Cancel' to back out of making changes.
      image
  • ⚠️ NOTE 1: it's still entirely possible to create a dropdown with 0 options, by simply creating a Dropdown Task and never editing the dropdown list. However, I'm thinking that this is a non-breaking edge case that only happens when a project owner didn't actually complete their workflow to begin with, so it's an "OK, I guess?" from me.
  • ⚠️ NOTE 2: when editing the dropdown list, the "Countries" preset will create 249 options, which means the modal can't be closed/saved.

Functionality tests & code changes look pretty good, though I did add a small suggestion about wrapping those number (4, 200) into some variables for easier maintenance in the future.

Testing

Tested with macOS + Chrome on staging URL, with a typical test project

Base functionality, in PFE Lab (?pfeLab=true):

  • User can create & delete Dropdown Tasks. (Assuming dropdown task experimental tool is enabled for project)
    • Dropdown Tasks are created with 1 dropdown list ("Main Dropdown") with 0 options.
  • User can make changes to the Dropdown Task's main questions/help text/etc, and changes are saved immediately.
  • User can edit dropdown lists:
    • they can change the list's title, required flag, etc
    • they can add/delete/rearrange options in the list
    • they can use Presets to quickly fill the list of options.
  • 🍏 Dropdown lists require a manual save process (clicking "Save") to confirm changes.
    • "Save" only works if dropdown list is valid, meaning title can't be empty.
    • Clicking "cancel" reverts any changes.

Additional functionality, in FEM Lab (?femLab=true)

  • 🍎 Dropdown lists require a manual save process (clicking "Save") to confirm changes.
    • "Save" only works if dropdown list is valid, meaning title can't be empty AND number of options must be between 4 and 200 (inclusive).
    • Clicking "cancel" reverts any changes.

Status

LGTM, but I have two non-blocking requests before you merge this.

  • First, please consider the small suggestion of turning those 4/200 numbers into more manageable vars/consts.
  • Second, please check with Cliff(? Sam?) as to why the maximum number of 200, since it clashes with the 249 countries preset.
    • If 200 is a hard limit, then I'd recommend removing/hiding the countries preset from the presets list, when femLab=true
    • If 200 is just an arbitrary number, maybe crank that up to 250?

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

I'm in favor of increasing the max to 250 to support the country dropdown, since it's a common use case for this tool.

@kieftrav kieftrav force-pushed the simple-dropdown-limit branch from 3a4ca25 to 996fc48 Compare October 12, 2023 15:37
@kieftrav kieftrav force-pushed the simple-dropdown-limit branch from 996fc48 to 668a3ae Compare October 12, 2023 15:38
@kieftrav kieftrav merged commit 7a3c8db into master Oct 12, 2023
@kieftrav kieftrav deleted the simple-dropdown-limit branch October 12, 2023 16:00
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.

6 participants