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

Show only breaking participants in check-in screen #2556

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

DDH13
Copy link
Contributor

@DDH13 DDH13 commented Dec 8, 2024

Fixes #2475

@DDH13
Copy link
Contributor Author

DDH13 commented Dec 8, 2024

I'm having a bit of a trouble reducing the cyclomatic complexity of the get_context_data method

Copy link
Member

@tienne-B tienne-B left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for this PR! Don't need to worry about Codeclimate BTW.

I was expecting more that the filter is added as an option on the top bar of the page, so it would be more of a frontend change than backend. Having it as a frontend filter would be more flexible, and could include adjudicators as well.

Screenshot 2024-12-08 at 11 07 54

However, that would also require a backend change to pass the teams' break(s) to the frontend. Does this sound like a good plan?

@DDH13
Copy link
Contributor Author

DDH13 commented Dec 8, 2024

Hey! Thanks for this PR! Don't need to worry about Codeclimate BTW.

I was expecting more that the filter is added as an option on the top bar of the page, so it would be more of a frontend change than backend. Having it as a frontend filter would be more flexible, and could include adjudicators as well.

Screenshot 2024-12-08 at 11 07 54 However, that would also require a backend change to pass the teams' break(s) to the frontend. Does this sound like a good plan?

Yep, will try to do what you said.

@DDH13
Copy link
Contributor Author

DDH13 commented Dec 13, 2024

@tienne-B
image
Was this what you had in mind?
I made the relevant backend change to pass information regarding breaking teams, speakers & adjudicators.
Struggling a bit with the frontend change since I'm not experienced with Vue.js. For now, it should console.log the breaking participants until I find out a fix to the bugs

Copy link
Member

@tienne-B tienne-B left a comment

Choose a reason for hiding this comment

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

It's advancing well! Little problem is that I'm getting these errors when trying to filter all (both adj and teams) by breaking:

Screenshot 2024-12-21 at 22 49 55

Copy link
Member

Choose a reason for hiding this comment

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

We'd have to set breaking_team_short_names for CheckInVenuesStatusView as well

Comment on lines +269 to +274
console.log('Breaking Teams and Adjudicators')
// print each object in the array
const temp = this.entitiesByPresence.filter(isBreaking)
for (let i = 0; i < temp.length; i++) {
console.log(temp[i])
}
Copy link
Member

Choose a reason for hiding this comment

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

Avoid using console, as it's just for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it's just temporary, will remove

const isBreaking = (entityByPresence) => {
if (entityByPresence.type === 'Adjudicator') {
return entityByPresence.breaking
} else if (this.breakingTeamShortNames.includes(entityByPresence.name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could using team IDs be shorter and avoid needing to escape the strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

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.

Create option to show only breaking participants in checkin screen
2 participants