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

Vacant division fixes #287

Merged
merged 10 commits into from Oct 18, 2023
Merged

Vacant division fixes #287

merged 10 commits into from Oct 18, 2023

Conversation

vvi230714
Copy link
Collaborator

Updates to show Vacant Divisions for all wards and only show included divisions for sub wards

// Check for sub division id in ward leader data and add placeholder if missing
let personData = commPersons.find(c => c.id == subDivisionId)
if (personData == undefined) {
personData = JSON.parse(JSON.stringify(personDefault))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this "deep clone"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I see - this is for the default. could just create an object... let's talk

@@ -293,6 +293,69 @@ export default {
]),
sampleBallotFormPrefilled () {
return `${SAMPLE_BALLOT_FORM}?ward=${this.ward}&party=${this.party}`
},
allCommitteePersons: function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

very minor, you can write this in the short hand style like the one above:

allCommitteePersons() {

// Get all divisions from ward boundaries
let allDivisions = this.wardBoundaries.features.map(x => x.properties.division)
// Default committe person data for Vacant divisions
let personDefault = function (ward, party) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor, this can be a const instead of let

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if you make it an arrow function (and maybe rename as vacantPerson()) you could use this.ward and this.party instead of taking them as params and take in the division and subdivision as arguments so you could have code below more like:

committeePersonsList.push(personData || vacantPerson(subDivisionId, division, subDivision));

}
let wardString = wardName.toString().padStart(2, '0')
let partyString = this.party.slice(0, 3).toUpperCase()
for (let a in allDivisions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize these are trivial variables, but you also use a and b as arguments in the sort below, which could lead to confusion. Maybe make the variable names a little more expressive like
for (let div in allDivisions) {

return this.wardBoundaries.features.length * 2
},
vacanciesCount () {
let vacantDivisions = this.allCommitteePersons.filter((p) => p.fullName === 'VACANT')
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor, you don't need the intermediate variable I think you can just do:
return this.allCommitteePersons.filter((p) => p.fullName === 'VACANT').length;

@pwolanin pwolanin merged commit fddf212 into master Oct 18, 2023
1 check passed
@pwolanin pwolanin deleted the Vacant-Division-Fixes branch October 18, 2023 00:39
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

2 participants