hackforla / tdm-calculator

DTLA Hack for LA is partnering with Los Angeles Department of Transportation (LADOT) to develop a Traffic Demand Management (TDM) calculator tool. This tool will help planners at LADOT and real estate developers to meet the Los Angeles’s Mobility Plan goals by 2035.
https://tdm.ladot.lacity.org
GNU General Public License v2.0
49 stars 33 forks source link

Implement tag system to allow for multiple AIN's to be entered on Page 1 #1010

Closed KPHowley closed 2 years ago

KPHowley commented 2 years ago

Overview

Allow multiple AIN's to be entered in the text box for AIN #, by implementing a Tag system.

Action Items

Target Features

Resources/Instructions

Hi-fi prototype - edit

See mockup and prototype here: https://www.figma.com/file/nD9QK56Mzq7xNSaSUoeGx0/TDM-Calculator?node-id=4919%3A17250

KPHowley commented 2 years ago

@cduong5 -can you post the latest prototype you shared here.

cduong5 commented 2 years ago

New prototype: https://www.figma.com/file/nD9QK56Mzq7xNSaSUoeGx0/TDM-Calculator?node-id=4919%3A17250 Video of new prototype: https://www.awesomescreenshot.com/video/6355727?key=8c276657df3b090b4d32b5310cb51049

ExperimentsInHonesty commented 2 years ago

@fyliu Please provide update

  1. Progress
  2. Blockers
  3. Availability
  4. ETA
fyliu commented 2 years ago

I found a package (react select) that can potentially be made to do exactly what we need. My plan is to first hook up the data to it and then tweak the styling. No blockers I can spend maybe 4 hours on it this week ETA 2 weeks since I'm not sure if I can finish this week

fyliu commented 2 years ago
  1. Progress - I gained more understanding of the select component but didn't make much progress because of not being able to spend time on this.
  2. Blockers - I had problem getting the styling right. Need to understand it better. Need to figure out how to hook up the component's events to what we have in onInputChange in TdmCalculationContainer.
  3. Availability - I might not be able to work on this for the next week
  4. ETA - Maybe in 2 weeks
fyliu commented 2 years ago
  1. Progress - I tried to make progress and discovered I need to learn something first. I ended up researching how to create custom events
  2. Blockers - I need to know how to create and pass a custom event up the component tree
  3. Availability - I can work on it more Thursday and maybe next week
  4. ETA - depends on the blocker. Maybe in 2 weeks.
fyliu commented 2 years ago

I would like to demo the current state of the input field to get further inputs about styling and behavior.

  1. Progress -
    1. Connected the data part of the component. It can load and save the tags in the project as a comma separated string. Any input not turned into tags are lost if the user focuses away from the input field.
    2. Input mask is working but it needs to get the mask from the database. I can clean that code up later.
    3. No empty tag
    4. No duplicate tag
    5. Create tag when pressing enter, tab, comma
    6. Create tag when pressing any key after input is full
  2. Blockers -
    1. Clarification needed: I need an example of the message for when the description says "create a message to be displayed after tag has been entered".
    2. Dev question: Is there a way to pass jss style classes into subcomponents?
    3. I'm working on styling the tag input to look the same as other inputs.
  3. Availability - a couple days this week
  4. ETA - progress feels good for having this completed next week
fyliu commented 2 years ago

Notes after reading the current Figma design. I didn't know that the design was updated. I need to evaluate the feasibility of these requirements.

  1. The design wants to override the AIN box's error text with one generated by the tags inside it. So "AIN/APN field is required" could turn into "Each AIN/APN must be 10 digits". Probably doable but adds time and complexity to all the input fields
  2. The design wants to enable the red outline on the AIN box when displaying the tag error message. Same as 1
  3. The design wants to do tag validation and display the improper tags in red color. Done
  4. The design wants to create input fields inside the tags when the user clicks on them. This is not feasible since the tags are displays and don't contain editing logic. The way to achieve a similar result right now is to delete the tag and re-enter it in the input field. Not easy and creates a 2nd place to do inputs, likely overly complicated design
    1. It may be possible to click on a tag and move the contents of it into the input field. Just my suggestion if we really want to be able to edit existing tags. It may not be worth the extra work if multiple AIN will rarely be used.
  5. The design wants to show an "Add another" placeholder label in the input field at the point when a tag is created. The problem with this is that the design assumes the input field will lose focus at that point, which is not the case. Therefore, the "Add another" text would not be shown during the AIN entry. It can be shown when the input field loses focus, which happens after the user is finished adding AINs. I'm not sure the "Add another" label is really useful to have. Easy to do even though it probably doesn't make sense to have
fyliu commented 2 years ago
  1. Progress - I added some validation to the display tags and I'm working on the "Add another" placeholder text
  2. Blockers
    1. I lost 2 days due to my child being sick
    2. Still need to tweak styling to match other input fields, which wasn't worked on since last time.
    3. Need to discuss some of the items above that I just found out about. Some are not easy or doesn't make sense.
  3. Availability - 4 hours before the next meeting
  4. ETA - 2/23
fyliu commented 2 years ago
  1. Progress - placeholder text works, input cursor jumps to the last digit remaining tasks:
    • [x] style the input field
    • [x] display error message if the tags are invalid
  2. Blockers - I could use some help on CSS spacing. It's taking more time than it should
click left triangle to expand to view the problem ![2022-02-23 16 02 44 localhost 215226a98f75](https://user-images.githubusercontent.com/1160105/155431622-8f52ee3c-34d0-40b0-aa96-8b29e2e7eb27.png)
  1. Availability - 1 day this week and 1 day next week
  2. ETA - I'd give it a week but it could be shorter or longer
fyliu commented 2 years ago
  1. Progress - All done but I'm doing more testing
  2. Blockers - none
  3. Availability - A few more hours before 3/2
  4. ETA - Should be done by 3/2

@seenaiype Some screenshots hidden below

click to expand ![Screenshot from 2022-02-27 22-38-14](https://user-images.githubusercontent.com/1160105/155936951-8ac9c3dc-485b-413c-ad56-0e32864d9248.png) ![Screenshot from 2022-02-27 22-38-41](https://user-images.githubusercontent.com/1160105/155936955-318ddec2-a057-4f90-87d3-9ef0eb0d4257.png)
fyliu commented 2 years ago

Question: Should an invalid AIN tag cause navigation to be disabled?

KPHowley commented 2 years ago

@fyliu - if this is a required field (which it is) we should verify the length is correct on AIN

fyliu commented 2 years ago

From the meeting, we (@KPHowley, @Shaeeka, @fyliu) decided that it's better to not allow users to create invalid tags that they would then have to delete and re-enter with the correct number. We want to create only valid tags. The next button logic would stay the same, it'd be enabled if there's any tags are present (with other required inputs satisfied).

Browsers render a highlight around the active input fields. It makes the AIN input halo around the mask rather than the whole AIN box. We want to have it look consistent with the other input boxes.

ExperimentsInHonesty commented 2 years ago

@Shaeeka please make a new issue to create a work around for the highlighting of the ain cell. We decided that it was going to be solved by the numbers appearing outside of the box, and the box only having enter a new number in it. As soon as you press enter, it should move the numbers to a gray tag above and keep you in the box the box to enter again.

When you finish making the issue, please close this issue and add the number of the new issue here

fyliu commented 2 years ago

My fault for tagging the pull request to close the issue. The pull request was all the functionality minus the last bit of UI tweaking for the input focus we talked about at a previous meeting.

It seems like the problem is in the Input Mask component taking that focus. The React Select component displays the input focus behavior the same way the design specified. I tried for a while and it feels like there's no way to make it work like the original design if we want to keep using Input Mask.

The workaround that Bonnie mentioned could be a viable way to do this. We just make Input Mask into the input field and let it take the input focus. We remove the original outline of the React Select component and display the AIN values if they're available. The only thing is collapsing the empty space when there's no AIN value yet.

Anyway, these are my thoughts on how to continue with this.

fyliu commented 2 years ago
  1. Progress - Separated the AIN values and the input field. Need to expand the input border to fill up the entire line
  2. Blockers - None
  3. Availability - 4 hrs before next team meeting
  4. ETA - 3/23 next meeting
fyliu commented 2 years ago
  1. Progress - The AIN values are now above the input field. The input field highlight shows up properly.
  2. Blockers - None
  3. Availability - 4hrs before next meeting
  4. ETA - should be done unless there are changes requested
fyliu commented 2 years ago

At the meeting, we decided to do these updates

fyliu commented 2 years ago

I did the fixes discusses yesterday but the following is not resolved and will take longer than today's merge deadline.

Question: Is it okay if the Next page button is not disabled when there's an error in the AIN input?

Question 2: Should the AIN length error or the empty input error have priority?

fyliu commented 2 years ago

From the meeting tonight.

  1. Don't make any more changes (releases) before user testing is done
  2. There's a bug that display the 10 digits error message when the input field is empty
  3. Answer to question 1: We do want to have the AIN input error about 10 digits to disable the Next button
  4. Answer to question 2: The way things currently are is fine. When there's no complete AIN and there's partially filled AIN input, it should show the error message about each AIN needing 10 digits
  5. The solid red error border for the AIN input box is okay (I pointed out it's a different style than the other error borders)
fyliu commented 2 years ago

Fixes:

New problem:

fyliu commented 2 years ago

Update: the backspace doesn't delete AIN problem has to do with the React Input Mask component stopping propagation of the backspace event. I don't know if that's a thing that all inputs do, but the React Select component relies on the event to work correctly. Our choices are these:

  1. Don't use the input mask. Use the input that comes with React Select that works correctly
    1. no input mask, which I don't think we want
  2. Replace the input mask with something we make ourselves
    1. remaking the component takes time and probably a lot of debugging
  3. Modify the input mask code and use that
    1. adding the whole code into our code base has licensing issues
    2. modifying and publishing the code under another name on npm solves the licensing issues
      1. there may be maintenance work required if the upstream gets updates
  4. Continue looking for another alternative

The above are the options and the drawbacks of each. This is a big issue for keyboard-only usage. It's not a problem if the user can use a mouse pointer.

Biuwa commented 2 years ago

Please confirm if this issue will be continuing or if a new issue replaces this @fyliu @KPHowley