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

13309 Add ability to clear LU form dropdowns #866

Merged
merged 5 commits into from Nov 17, 2020

Conversation

godfreyyeung
Copy link
Contributor

@godfreyyeung godfreyyeung commented Nov 16, 2020

Summary

Allows users to clear the dropdown selection on the Land Use Form Page. Including for these questions:

1 PAS/RWCDS/Land Use Form > applicant geographic state
2 Land Use Form > Zoning Map Amendments section > Total Area of All Zoning Lots
3 Land Use Form > Zoning Map Amendments section > Existing Zoning District
4. Land Use Form > Environmental Review section > Lead Agency
5. Land Use Form > Proposed Actions section > Previously Approved Action code
6. Land Use Form > Proposed Actions section > Zoning Resolution action is Pursuant to

Task/Bug Number

Fixes AB#13309

Technical Explanation

  • For questions 1-3 above, the solution was simply to set the @allowClear={{true}} argument on the Power Selects. This enables a Power Select built-in feature where a user can click an "x" to assign the bound property a null value. This worked because those properties are simple text or integer values.

  • For questions 4-6 above, the solution was more complex because they deal with models/objects. Also, the power selects in 4-5 read the options from one model and assign the selected option to another ("target") model. They currently do so by assigning the selected option's ID to an auxiliary, made-up "*chosenId" field on the target model, and relying on the backend to read that chosenId and patch relationships accordingly. Thus, this PR follows that example, and modifies the backend to delete relationships according to the incoming value of the "*chosenId" fields.

This whole workflow should be reworked in the future. Ideally, these powerselects read and write to the same model, or polymorphic models. The frontend should associate and disassociate models based on the dropdown selections, so that when the form is saved, the proper JSONAPI compliant requests are generated.

  • Sets up a new disassociateHasOne() method on the CRM Service for disconnecting an 1:1 entity relationship

  • Patches up a few Power Select components so that the concerned property is displayed as the selected value on load. Previously they displayed the value as the placeholder, so it would appear gray instead of black.

  • Another future todo: Questions 1-3 and 4-6 have different styles for the "clear" button. One is a black "x", the other is the text "clear". we should design a consistent style for all dropdowns.

@godfreyyeung godfreyyeung requested a review from a team as a code owner November 16, 2020 11:18
@godfreyyeung godfreyyeung force-pushed the 13309-clear-dropdowns branch 2 times, most recently from 39dcff0 to c4573c5 Compare November 16, 2020 22:40
@codecov
Copy link

codecov bot commented Nov 16, 2020

Codecov Report

Merging #866 (c4573c5) into develop (2308c29) will decrease coverage by 1.03%.
The diff coverage is 61.53%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #866      +/-   ##
===========================================
- Coverage    85.66%   84.63%   -1.04%     
===========================================
  Files          114      113       -1     
  Lines          900      911      +11     
===========================================
  Hits           771      771              
- Misses         129      140      +11     
Impacted Files Coverage Δ
...ackages/landuse-form/zoning-resolution-dropdown.js 50.00% <50.00%> (-25.00%) ⬇️
...ts/packages/landuse-form/proposed-action-editor.js 68.42% <60.00%> (-11.58%) ⬇️
...pp/components/packages/landuse-form/lead-agency.js 69.23% <75.00%> (-19.66%) ⬇️
client/app/components/project/package-section.js 6.66% <0.00%> (-13.34%) ⬇️
client/app/models/package.js 95.34% <0.00%> (ø)
client/app/helpers/to-locale-string.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2308c29...4a94b1c. Read the comment docs.

Copy link
Contributor

@trbmcginnis trbmcginnis left a comment

Choose a reason for hiding this comment

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

Awesome! Your PR description is so thorough and amazing, I love it.

I left some comments for just removing some code that was rendered obsolete by your changes (just a little bit of clean-up, nothing major), but other than that it looks great, thanks!

Copy link
Collaborator

@allthesignals allthesignals left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!! Just left a few questions.

Code is a bit ugly because of how the PowerSelect was set up and the multiple models it deals with. We should take time to rework this.
Specifically the questions about "total area of all zoning lots to be rezoned" and "Existing zoning districts"
The dropdown is part of the Proposed Action Editor
@godfreyyeung godfreyyeung merged commit e2d5dc2 into develop Nov 17, 2020
@godfreyyeung godfreyyeung deleted the 13309-clear-dropdowns branch November 17, 2020 17:46
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.

None yet

3 participants