-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Feature: Interview Survey Form with tom-select
#5165
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
| } | ||
|
|
||
| @import 'tom-select/dist/css/tom-select' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For whatever reason, this import was only working for me when placed at the bottom of the file. Maybe something to play around with later.
| @@ -1,12 +1,31 @@ | |||
| class InterviewSurveysController < ApplicationController | |||
| before_action :authenticate_user! | |||
| # TODO: merge feature flag stuff in | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After #5164 is merged, I'll be able to clean some of this stuff up.
| connect () { | ||
| this.tomSelect = new TomSelect( | ||
| this.element, | ||
| { create: true } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially going to be more to do here. Kind of depends on how the feature scales up. One problem might be that if there are hundreds of concepts, loading them upfront to be searched through might have performance issues. We'd have to probably create a rails controller for searching concepts and then configure tom-select's search input hit that controller. That's doable of course, but it'd mean adjusting more of this config.
| attribute :interview_concept_names, :string, array: true, default: -> { [] } | ||
|
|
||
| after_save :create_concepts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Willing to accept some feedback on how this is wired up. I generally don't like using AR callbacks much, and it might be better to just call something from the controller.
I do like this virtual attribute though, and the way this is done ensures that if validation fails for some reason (like the user enters a bad date or doesn't enter a date at all), the multi select will still be populated with the items they selected.
| find('div[class="ts-control"]').click | ||
| find('input[id="interview_survey_interview_concept_names-ts-control"]') | ||
| .set("React props") | ||
| .send_keys(:return) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a page object could help with some of this test readability.
| validates :name, presence: true | ||
| validates :name, length: { minimum: 2, maximum: 25 } | ||
|
|
||
| normalizes :name, with: ->(name) { name.downcase } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new way to solve the problem given in this comment of the previous PR: #5018 (comment)
Basically this just means that every concept will have its name case normalized to lowercase on saving. I think this is likely a good idea to combat duplicates. We might also want to make this field unique (it's not currently)?
|
TODO:
|
Because
We need a multi select form on interview surveys.
Note that this intentionally has a fairly constrained scope just to make sure we have the general behavior down for what we want. The form still looks kind of bad because little attention was paid to styling, validations aren't completely ironed out, and the feature is still hidden behind a feature flag.
This PR
tom-selectlibrary (wrapped in a Stimulus controller)Issue
Closes #4966
Additional Information
This ended up being more than a spike lol. Might change the branch name later.
Pull Request Requirements
keyword: brief description of changeformat, using one of the following keywords:Feature- adds new or amends existing user-facing behaviorChore- changes that have no user-facing value, refactors, dependency bumps, etcFix- bug fixesBecausesection summarizes the reason for this PRThis PRsection has a bullet point list describing the changes in this PRIssuesection