-
Notifications
You must be signed in to change notification settings - Fork 3
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
Circle packer 3.0!! #21
Circle packer 3.0!! #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR honestly looks good, but I'm super sick right now so I'm going to get someone else to review it too. I didn't check any of your math, you know a lot more about it than I do at the moment, also I think you're TS skills are Proficient + but I need confirmation that this actually works and there are some things that give me cause for concern. I would test it on my device, but I don't have it setup with all the CLUCK config thingies and don't feel like doing so while I'm sick. When either the other reviewer or yourself shows me a video of this working, I'll fully support a merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main things are what @Ewie21 and I wrote on slack:
- Member circles should be closer together
- Clock circle should be bigger
- Member circles also shouldn’t jump back to their original position when they’re re-rendered.
- Size disparity too extreme
I'd also like to caution against leaving in code and comments that don't make sense or that don't have a purpose just because they were there before.
I don't think everything was addressed in your latest commit, I left reminders in the relevant conversations. Also, was busBubble.ts being used at all, or was it just duplicate code? If it was duplicate code, is there a benefit in your opinion to keeping it extracted in a separate file? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through and resolved the comments that were no longer relevant, but there are still 3 left that need changes or further discussion
No description provided.