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

feature request: update blip drawn coordinates to be in ascending order within rings #311

Closed
setchy opened this issue Apr 22, 2023 · 11 comments · Fixed by #322
Closed

feature request: update blip drawn coordinates to be in ascending order within rings #311

setchy opened this issue Apr 22, 2023 · 11 comments · Fixed by #322

Comments

@setchy
Copy link
Contributor

setchy commented Apr 22, 2023

I noticed there is a difference between the order / placement of blips when comparing the latest official Thoughtworks Tech Radar - Vol 27, vs the BYOR Radar using the Vol 27 sample dataset

On the official Thoughtworks Tech Radar the blips are nicely drawn in ascending order from left to right within each ring.
Screenshot 2023-04-22 at 4 12 38 PM

While on the BYOR version, the blips are randomly drawn within each ring.
Screenshot 2023-04-22 at 4 13 14 PM

This random placement within the BYOR version makes it harder for users to locate the blips.

This issue is to update the BYOR implementation to match that of the thoughtworks.com/radar and make the drawn positions more predictable / orderly.

@setchy setchy changed the title feature request: update blip drawn coordinates to be ascending within rings feature request: update blip drawn coordinates to be in ascending order within rings Apr 23, 2023
@setchy
Copy link
Contributor Author

setchy commented Apr 23, 2023

I was going back through the Tech Radar PDF reports archive earlier today, and it seems that this ordered behavior was first introduced in the official reports from Volume 10 onwards.

@marisahoenig
Copy link
Member

Thanks for this, @setchy! This is on my "radar" too in terms of new features. I'll chat with the team about it.
Cheers,
Marisa

@setchy
Copy link
Contributor Author

setchy commented Apr 26, 2023

Thank you so much @marisahoenig!

@marisahoenig
Copy link
Member

Hey @setchy! Following up on this, as you noted, we have this behavior in the official TW Radar. I'm going to explore how easy it is to carry over that implementation into BYOR and try to implement that. But, it's worth noting that the rest of the team is currently working on other features for the Tech Radar and this is not at the top of my list at the moment, so it may be a few weeks until we can get this update in. I'll keep you updated as I work on it. Cheers!

@setchy
Copy link
Contributor Author

setchy commented May 3, 2023

Thanks @marisahoenig for the update.

If there is anything I can assist with, please let me know.

Otherwise, I'll keep my eyes out for further updates 👀 😄

@setchy
Copy link
Contributor Author

setchy commented Jun 16, 2023

Hi @marisahoenig - any hints on what logic changes might be required to support this ordering of blips? Happy to lend a coding hand to implement if I can be pointed in the direction used by the TW public radar

@marisahoenig
Copy link
Member

Hey @setchy. Sorry for the delay. It would be awesome if you have time to work on an implementation. I realized our internal codebase and BYOR have some underlying differences, all the way down to the programming language (internally, it's in Ruby). So, the implementation will need to be different regardless.

Before we display the Radar, we need to sort the blips by quadrant and then by ring (the final list should be alphabetized within each ring). Internally, we sort the blips and then put them all in a JSON to display. For BYOR, users have the option to use a JSON, which I think would be the perfect use case for this. However, it would be great to make it work for the CSVs/Google Sheets too.

I think this logic may need to live in the radar.js file in BYOR but I'm not positive — I haven't spent enough time deep in the code. I've unfortunately had very limited bandwidth for BYOR recently, so any help you can give is much appreciated. Please let me know if you have further questions/suggestions.

@setchy
Copy link
Contributor Author

setchy commented Jul 7, 2023

I've had a quick POC and although I've sorted the blips alphabetically, I sense there is more too this.

My hunch is that it's within the way the blip coordinates are calculated - ie:

function calculateRadarBlipCoordinates(minRadius, maxRadius, startAngle, quadrantOrder, chance, blip) {

I've tinkered with different radius and angle values and that seems to have a bearing...

Is there anything special about how the internal implementation handles blip coordinate and collision detection?

@marisahoenig
Copy link
Member

Oh, super interesting. Yes, this definitely has an impact. Internally, we automatically figure out the theta of each blip, but we manually control how close/far a blip is from the center of the Radar (the radius). This is because how far/close the blip is actually has some meaning for our volumes. However, it looks like we have the location automatically randomized for BYOR — both the theta and radius.

I think BYOR would need to:

  1. Figure out the alphabetization
  2. Based on the number of blips in a ring in one quadrant, equally space out the blips in order, theta wise.
  3. Then, randomize the radius for each blip.

@setchy
Copy link
Contributor Author

setchy commented Jul 8, 2023

PR #322 raised for drawing the blips in numerical order

@setchy
Copy link
Contributor Author

setchy commented Jul 8, 2023

I've also split the alphabetic sorting of blip names into #330

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 a pull request may close this issue.

2 participants