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

Improve SitesNeighbourhoods selector #887

Merged
merged 17 commits into from
Feb 1, 2022

Conversation

lexiwitch
Copy link
Contributor

@lexiwitch lexiwitch commented Jan 19, 2022

Improves Secondary Neighbourhood selection in Sites Admin view
Fixes #885

  • Changed sites/_form.html.erb to use Cocoon
    • Added sites/_sites_neighbourhood_fields.html.erb subform
    • Added helpers/sites_helper.rb for Sites Neighbourhood collection
    • Added @nathvda/cocoon
    • Added behaviours.site.js (for select2 single option select)
    • Added contextual_name() in Neighbourhoods model
    • Added name() in SitesNeighbourhood model
  • Migrate Neighbourhoods to memoize parent.name to avoid complex lookup

@lexiwitch lexiwitch requested a review from kimadactyl January 19, 2022 23:07
@kimadactyl kimadactyl added enhancement patch Bump version 0.0.X labels Jan 24, 2022
@lexiwitch lexiwitch force-pushed the 885_sites_neighbourhoods_selector branch from a254c82 to 479b2b4 Compare January 25, 2022 14:58
@lexiwitch lexiwitch marked this pull request as ready for review January 25, 2022 14:59
@lexiwitch lexiwitch changed the title [WIP] #885 sites neighbourhoods selector #885 sites neighbourhoods selector Jan 25, 2022
@lexiwitch lexiwitch requested a review from katjam January 25, 2022 15:29
@lexiwitch lexiwitch requested a review from kimadactyl January 25, 2022 18:38
Copy link
Member

@kimadactyl kimadactyl left a comment

Choose a reason for hiding this comment

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

See comments

@lexiwitch lexiwitch requested a review from kimadactyl January 26, 2022 14:32
@kimadactyl kimadactyl changed the title #885 sites neighbourhoods selector Improve SitesNeighbourhoods selector Jan 26, 2022
@kimadactyl
Copy link
Member

Is this loading neighbourhoods via ajax?

Copy link
Member

@kimadactyl kimadactyl left a comment

Choose a reason for hiding this comment

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

  • Make fuzzy search type input work on new selects
  • All selects currently set to Abbey, Leics
  • Make delete link just a link and tidy up UX a lil. i would put all of the inputs together in a fieldset
  • Think we need to do something to make the site save buttons look bigger / more important than the secondary neighbourhoods buttons

@kimadactyl kimadactyl added minor and removed patch Bump version 0.0.X labels Jan 26, 2022
lexiwitch and others added 9 commits February 1, 2022 13:18
… creation. So it is intended to be scrapped for another solution
…oods (update fails due to it still passing the neighbourhood_ids parameter l o l. so it is useless. Also probably singlehandedly the most disgusting code ive written lmaoo
- Memoize parent_name in Neighbourhoods table
  - Add that to the Neighbourhoods datatables
- Add try/catch block to datatable.js to stop errors appearing on pages
  that do not contain datatables
- Remove filtering of primary id from options_for_sites_neighbourhoods
- Correct Sites behaviours file to:
  - Properly add select2 to sites neighbourhoods relational inputs
  - Remove primary neighbourhood from form field input
- Ensure primary neighbourhood selection uses select2
- Properly render SitesNeighbourhoods relation input form template
@lexiwitch lexiwitch force-pushed the 885_sites_neighbourhoods_selector branch from 7fe7236 to 114896d Compare February 1, 2022 13:23
@lexiwitch
Copy link
Contributor Author

Rebased on main

@lexiwitch lexiwitch requested a review from kimadactyl February 1, 2022 13:23
Copy link
Member

@kimadactyl kimadactyl left a comment

Choose a reason for hiding this comment

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

  • A few UX suggetions as per discord
  • Add better description text under headings
  • Fix create new site picker for main neighbourhood

@lexiwitch
Copy link
Contributor Author

  • A few UX suggetions as per discord
  • Add better description text under headings

Done :)

  • Fix create new site picker for main neighbourhood

hmm it is not broken for me, it just doesn't display as a select2 element for whatever reason. Moved it to a f.input element so the rendering is fixed

@lexiwitch lexiwitch requested a review from kimadactyl February 1, 2022 15:28
Copy link
Member

@kimadactyl kimadactyl left a comment

Choose a reason for hiding this comment

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

Suggested controller tests:

  • Create new site with primary neighbourhood only saves. Ditto for edit/update
  • Create new site with both primary and secondary neighbourhood saves. Ditto for edit/update.
  • Create new site with primary neighbourhood == secondary neighbourhood just saves the primary neighbourhood. Ditto for edit/update
  • Same as above but with a few secondary neighbourhoods
  • Primary neighbourhood is mandatory (might exist already)

@kimadactyl kimadactyl merged commit 6be0e5e into main Feb 1, 2022
@kimadactyl
Copy link
Member

Merged, controller tests created as another ticket

@kimadactyl kimadactyl deleted the 885_sites_neighbourhoods_selector branch February 1, 2022 17:25
ivankocienski pushed a commit to ivankocienski/PlaceCal that referenced this pull request Feb 2, 2022
* Add select2 to primary neighbourhoods form
* Add select2 + cocoon to secondary neighbourhoods form
* Memoize parent_name field in Neighbourhoods
* Give neighbourhoods more context
* Add try/catch block to datatable.js to stop errors appearing on pages that do not contain datatables
* Remove filtering of primary id from options_for_sites_neighbourhoods
* Correct Sites behaviours file to remove primary neighbourhood from form field input
* Fix CSS on Cocoon
* Buttons: Red. Descriptions: Added

Co-authored-by: Dr Kim Foale <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants