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: add optional support for moved in / out blips #310

Merged
merged 19 commits into from
May 16, 2024

Conversation

setchy
Copy link
Contributor

@setchy setchy commented Apr 22, 2023

Hot on the heels of the v1.0.0 launch this week, I thought I'd see whether I could self-serve #177

Adds optional support for moved in and out blip drawing.

Below are two datasets for Thoughtworks Volume 27 sample (csv) that can be used to demonstrate/test:

Screenshot 2023-04-22 at 4 18 08 PM

@setchy
Copy link
Contributor Author

setchy commented Apr 25, 2023

@devansh-sharma-tw @naveengenupuritw - appreciate your feedback re this feature proposal

@setchy
Copy link
Contributor Author

setchy commented Apr 27, 2023

@marisahoenig - appreciate any feedback from the team re this PR 🙏

@devansh-sharma-tw
Copy link
Contributor

Thanks for raising this @setchy ! We're internally discussing on adding the moved in/out status as well (to keep BYOR inline with our Thoughtworks Radar).
This PR helps a lot for this and we'll update here soon regarding the feature.
Thanks!

@setchy
Copy link
Contributor Author

setchy commented Jun 16, 2023

@devansh-sharma-tw - hope all is well. Just checking in to see if your team has had a chance to review and consider this PR.

Copy link
Member

@marisahoenig marisahoenig left a comment

Choose a reason for hiding this comment

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

Adding some suggestions. I think it's worth adding one new field and letting users have the option to use it OR the isNew field. Would love your thoughts @setchy

spec/util/inputSanitizer-spec.js Outdated Show resolved Hide resolved
src/graphing/blips.js Outdated Show resolved Hide resolved
src/stylesheets/_quadrants.scss Show resolved Hide resolved
@setchy
Copy link
Contributor Author

setchy commented Jul 8, 2023

@marisahoenig - I have updated this PR to support a new, optional column/field called status.

Accepted values are case-insensitive new, no change, moved in, moved out. I welcome any feedback on alternate option names (eg: existing or unchanged etc)

To demonstrate, I've updated my TW volume datasets, eg TW Volume 28.

All existing datasets using isNew should continue to work.

@setchy
Copy link
Contributor Author

setchy commented Sep 7, 2023

@marisahoenig @devansh-sharma-tw - any feedback re: this PR?

@cvium
Copy link

cvium commented Dec 19, 2023

This seems like a very nice addition. A shame it's been sitting for so long

@setchy setchy requested a review from marisahoenig March 31, 2024 10:40
@setchy
Copy link
Contributor Author

setchy commented Apr 1, 2024

Note

In the interim I've deployed my BYOR enhancements @ https://radar.setchy.io, which bring it up to the functionality found on https://thoughtworks.com/radar

@danielkoch
Copy link

@marisahoenig: Any plans or update on this one?

Copy link
Member

@marisahoenig marisahoenig left a comment

Choose a reason for hiding this comment

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

Hey @setchy I left a few comments. Please resolve and also make sure the most recent code from the main branch of this repo is loaded in. I'll chat with the team next week to see if we can get this added. The slowdown was related to the most recent Radar release, and we need to make sure changes like this are reflected on our live site as well. Apologies for the delay — it's a great add to the tool.

spec/graphing/blips-spec.js Outdated Show resolved Hide resolved
spec/graphing/blips-spec.js Outdated Show resolved Hide resolved
spec/graphing/blips-spec.js Outdated Show resolved Hide resolved
src/graphing/radar.js Outdated Show resolved Hide resolved
src/graphing/radar.js Outdated Show resolved Hide resolved
spec/models/blip-spec.js Show resolved Hide resolved
src/graphing/blips.js Outdated Show resolved Hide resolved
src/graphing/blips.js Outdated Show resolved Hide resolved
@setchy setchy requested review from will-amaral and a team as code owners April 10, 2024 22:47
@setchy
Copy link
Contributor Author

setchy commented Apr 10, 2024

Hey @setchy I left a few comments. Please resolve and also make sure the most recent code from the main branch of this repo is loaded in. I'll chat with the team next week to see if we can get this added. The slowdown was related to the most recent Radar release, and we need to make sure changes like this are reflected on our live site as well. Apologies for the delay — it's a great add to the tool.

Thanks @marisahoenig, appreciate the feedback. I've gone through and addressed each of them 😄.

No problems about the delay, too - completely understand.

Copy link
Member

@marisahoenig marisahoenig left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes, @setchy! As mentioned, I'll chat with the team next week about getting this incorporated and seeing if there is anything else that needs to be updated on the team's end.

@setchy
Copy link
Contributor Author

setchy commented May 9, 2024

@will-amaral - appreciate your thoughts on this PR

@shiviraj shiviraj merged commit e82ed6f into thoughtworks:master May 16, 2024
2 checks passed
@setchy
Copy link
Contributor Author

setchy commented May 16, 2024

Thank you @shiviraj @marisahoenig @will-amaral - what a great gift for a Thursday morning 🙏

@setchy setchy deleted the feature/177-moved-in-out branch May 16, 2024 13:20
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.

6 participants