Skip to content

feature: add optional support for moved in / out blips#310

Merged
shiviraj merged 19 commits into
thoughtworks:masterfrom
setchy:feature/177-moved-in-out
May 16, 2024
Merged

feature: add optional support for moved in / out blips#310
shiviraj merged 19 commits into
thoughtworks:masterfrom
setchy:feature/177-moved-in-out

Conversation

@setchy

@setchy setchy commented Apr 22, 2023

Copy link
Copy Markdown
Contributor

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

setchy commented Apr 25, 2023

Copy link
Copy Markdown
Contributor Author

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

@setchy

setchy commented Apr 27, 2023

Copy link
Copy Markdown
Contributor Author

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

@devansh-sharma-tw

Copy link
Copy Markdown
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

setchy commented Jun 16, 2023

Copy link
Copy Markdown
Contributor Author

@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.

@marisahoenig marisahoenig left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread spec/util/inputSanitizer-spec.js Outdated
Comment thread src/graphing/blips.js Outdated
Comment thread src/stylesheets/_quadrants.scss
@setchy

setchy commented Jul 8, 2023

Copy link
Copy Markdown
Contributor Author

@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

setchy commented Sep 7, 2023

Copy link
Copy Markdown
Contributor Author

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

@cvium

cvium commented Dec 19, 2023

Copy link
Copy Markdown

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

setchy commented Apr 1, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown

@marisahoenig: Any plans or update on this one?

@marisahoenig marisahoenig left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread spec/graphing/blips-spec.js Outdated
Comment thread spec/graphing/blips-spec.js Outdated
Comment thread spec/graphing/blips-spec.js Outdated
Comment thread src/graphing/radar.js Outdated
Comment thread src/graphing/radar.js Outdated
Comment thread spec/models/blip-spec.js
Comment thread src/graphing/blips.js Outdated
Comment thread src/graphing/blips.js Outdated
@setchy setchy requested review from a team and will-amaral as code owners April 10, 2024 22:47
@setchy

setchy commented Apr 10, 2024

Copy link
Copy Markdown
Contributor Author

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.

@marisahoenig marisahoenig left a comment

Copy link
Copy Markdown
Contributor

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

setchy commented May 9, 2024

Copy link
Copy Markdown
Contributor Author

@will-amaral - appreciate your thoughts on this PR

@shiviraj shiviraj merged commit e82ed6f into thoughtworks:master May 16, 2024
@setchy

setchy commented May 16, 2024

Copy link
Copy Markdown
Contributor Author

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