-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Publisher] Add "Other" textbox to race breakdown #1557
[Publisher] Add "Other" textbox to race breakdown #1557
Conversation
Hi @mxosman! So after playing with this around, I think maybe injecting component into Also, how do you like my implementation of And last one: I was thinking about displaying existing |
Ah, do you mind clarifying your thoughts on this? Is it moreso challenging to refactor the component to inject a text input? I could see us doing this more often - but just wanted to understand the challenges you ran into!
Hmm... the biggest thing I don't like about the textbox before is the big distance between 'Other' and the textbox - it makes it feel less intuitive what that input is connected to, despite the description in the box. Ideally, it makes the most sense from a UX perspective that it happens right after the 'Other' checkbox, but if that's too technically clunky, the next best option IMO is below the checkboxes because it keeps it within the closest proximity to the 'Other' checkbox. To help with it always being visible and to enhance its connection to the 'Other' checkbox, we could add a E.g. -
I think this works! It's hard to tell without the backend implemented b/c this is a complex component and the Other applies to 3 different sets of ethnicities, but reading the payload I think this works perfectly fine!
Ah, hmm... I might not be fully understanding - selecting an answer for |
<NewInput | ||
type="text" | ||
label="" | ||
placeholder={`If the listed categories do not adequately describe this breakdown, please describe additional definition/clarification of the "Other" selection.`} |
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.
Let's adjust this to just be Please describe additional definition/clarification of the "Other" selection.
Oh, one thing I realized I forgot to include in the ticket - can we make filling out the text field a requirement if a user is going to select "Other"? Meaning, if they enter nothing, we should keep "Other" unchecked. Sorry for the last minute complexity! |
…to nasaownsky/1491-race-breakdown-other-textbox
Hi @mxosman! I've updated textbox logic according to your comments, so please check out my changes and if it's working correctly! In the meantime I only have to find a way for this to work properly:
Thank you! |
Thanks, @nasaownsky - will dive into this tomorrow morning! |
Nice @nasaownsky! It looks like it's working exactly as intended in all 3 cases I tested. Thank you for updating the logic + positioning of it and working through the last piece. Almost there! I'll review once everything is in place. |
Hi @mxosman! Please, play with my latest changes and let me know what you think about my implementation! Does it fit your vision right? I've tried several different approaches and this one seemed to be the most suitable for our needs. |
Hi @mxosman! I think I actually need help with this. So, after playing with BE update @nichelle-hall provided I assume I still need to work with dimensions itself, because |
Hi @nasaownsky! I think I see the confusion here - you'll be writing to the context field in the layer above dimensions - the disaggregations. So, whatever the user enters in the "Other" Race & Ethnicities field, will be saved to and read from the Instead of this: |
Right, but then this disaggregation context has only spot for one "Other" description, and how should we write three different versions of it there? For |
We've simplified it from 3 separate fields to just that one field. So any input in the "Other" just goes in that one field - you no longer have to worry about 3 separate fields as this one will cover all 3. |
Hey @nichelle-hall! I've updated logic to save other description in disaggregations contexts, and it sends payload to but it seems like it is not saved in BE, because if I reload the page there is no value in the context: Could you please check this for me? I'm in your test env. Thank you so much in advance! |
Hi @nasaownsky - Nichelle is out until next Monday - but, I pulled in your latest changes and tried myself, and am not seeing the Screen.Recording.2025-01-02.at.3.44.16.PM.mov |
Hello @mxosman! Happy new year, hope you had great holidays! I'm also out til Wednesday, but decide to respond as I saw your message and I think I know what is going wrong for you specifically -- I think you still using default env url, try change that to |
Happy New Year to you, Ilya! Hope you had a lovely holiday as well - and are enjoying your time off (please don't feel obligated to respond on your days off). My environment was pointed to Nichelle's test URL - but the payload is coming from the frontend, and I'm not sure why I don't see it included in the PUT request. When you do |
Oh strange, I tried it again this morning and now I see the payload. Maybe I was in the wrong env after all, but could've sworn I adjusted it and re-ran before playing with it and recording. Sorry about that! Totally see what you're seeing - let's see what Nichelle thinks when she returns and go from there. In the meantime, I'll do a review at some point today since this should hopefully be mostly there. |
const [otherDescription, setOtherDescription] = useState( | ||
currentOtherDescription | ||
); | ||
const [isOtherChecked, setOtherChecked] = useState(Boolean(otherDescription)); |
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.
Maybe we should check for the presence of currentOtherDescription
instead of the otherDescription
state value for the initializer.
].contexts?.find((context) => context.key === "OTHER_RACE_DESCRIPTION"); | ||
|
||
if (otherDescription !== undefined) { | ||
if (otherDescriptionContext) { |
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.
nit: could we simplify and just do something like if (otherDescription && otherDescriptionContext)
@@ -463,6 +471,8 @@ class MetricConfigStore { | |||
this.dimensions[systemMetricKey][disaggregationKey][ | |||
dimensionKey | |||
].description = dimensionData.description; | |||
this.dimensions[systemMetricKey][disaggregationKey][dimensionKey].contexts = | |||
dimensionData.contexts; |
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 don't think we need to reference the dimension context anymore, right?
|
||
useEffect(() => { | ||
setOtherDescription(currentOtherDescription); | ||
}, [currentOtherDescription]); |
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'm wondering if we need this useEffect
? If currentOtherDescription
is defined inline, and used to initialize currentOtherDescription
, when do we expect the value of currentOtherDescription
to change for this to be useful? I worry we're going to run into some unexpected race conditions with these useEffects.
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.
Oh, that was necessary when we maintained three different other descriptions, now it could be removed. Thank you for pointing this out!
setOtherChecked(!checked); | ||
|
||
if (checked) { | ||
// Clear otherDescription if "Other" is unchecked |
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.
Thanks for the comment!
Hey Ilya, I was testing our code end-to-end and found an issue on my side, but also noticed an error on your end. It looks like the contexts aren’t being sent over the first time—only on the second attempt when I redo the request to add another description. I’ve uploaded a screen recording to illustrate the issue. Let me know your thoughts! Screen.Recording.2025-01-06.at.4.44.12.PM.movI've pushed up my bug fix to same link I sent earlier. |
Thank you so much for your feedback & fix, @nichelle-hall! I wonder if this is due to this line in the
Specifically the check for @nichelle-hall - do you know whether or not the backend sends an empty |
It didn't! But I just pushed up a fix so that now it does. That said, I'm still seeing the error. |
Gotcha - thank you, Nichelle! I just tried it again w/ a few agencies and it's working for me on the first go. Would you mind trying again and triple checking your |
I'm now seeing the data saved on the first edit! I must have been on the wrong BE branch. |
@mxosman @nichelle-hall Hi all! This is utterly strange but now this is not working for me at all. When I check |
You know what! I think I made a mistake and wrote over some changes on my playtesting branch. I'l redeploy now! |
@nichelle-hall It's working now, thank you! |
@mxosman thank you for great comments, now it should be ready! |
Whew - thank you for the changes! I was sooo close to approving, but ran into a strange issue specifically with Jails > Total Population Race & Ethnicity metric - for some reason this isn't sending a context object - can you take another look? [Super confirmed that I'm using the right - which is Nichelle's playtest link] Screen.Recording.2025-01-10.at.4.41.04.PM-Issue.mov@nichelle-hall - wondering if you can take a look too to see if you can repro and if there might be something in the BE related to this particular metric? |
Oh, that's unfortunate! I think that I can't test this issue, because the only way for me to test this in my environment is Jails (AR) agency (id: 151), but when I selecting it -- it is stuck in infinite loading with 500 error. Maybe the reason of this issue somewhere in BE? |
Hi @nasaownsky/@nichelle-hall - oh strange - there is something weird going on with that agency... I'm getting a server error too. You should still be able to test this issue with a different agency or by creating a new agency... Can you try that, please? You can point the environment to regular |
Oh, yes, I forgot that I could create one! Sure, I'll give it a shot. |
OK, hello again @nasaownsky & @nichelle-hall. I did some digging and it looks like for the metrics that are affected (metrics in Jails and Prisons, and probably some others I haven't tested yet) that the backend is not sending over the OTHER_RACE_DESCRIPTION context field. @nichelle-hall - I saw that you added these in to the TIGs in your #36052 PR - any hunches as to why it's not sending them over? You can try this agency: http://localhost:3000/agency/1693/metric-config?system=jails&metric=jails_total_population - if you look at the network response when you load the Total Daily Populations metric, it doesn't show anything with a @nasaownsky - is it necessary for the FE to have this field before it's able to send it in the PUT request? If not, is there a way that we can just always send this context field regardless of whether or not the backend sends it initially? |
@mxosman Thank you for your detailed investigation! I played around with implementation and I think we could eliminate this issue on FE side by introducing defaultOtherDescriptionContext object. Tested with newly created Jails agencies and it seems like the issue you describing is gone! |
Great call! I was hoping we could do exactly this - just send it anyway and not depend on the BE giving it to us. Thanks for the adjustment. One small issue I noticed - the "Other" doesn't check after you save: Screen.Recording.2025-01-14.at.10.11.51.AM.movCould you take another look and spend a bit more time manually testing different agencies/metrics to make sure things are working as intended? I recommend creating a new agency with Law Enforcement, Jails, Prisons, etc. sector to get a decent variety of cases. |
I know you have a plan of attack, but I'm also looking into a fix. The BE should be sending the contexts over! I merged a PR yesterday that made some changes regarding sending report contexts and I wonder if I introduced a bug. |
@mxosman Hmm, this one actually could be tricky without the context. Let me think about it some more. @nichelle-hall Oh, great to hear! I think it should fix that problem for good, with the context sending from BE. |
I've found the bug and pushed up the changes to |
Ah, amazing - that fixes everything! @nasaownsky & @nichelle-hall - thank you both SO MUCH for all your work on this - what a journey! I did another round of tests and I couldn't break it. I'm glad we're finally able to ship this! 👏 👏 👏 We'll wait for the backend fix to go through review and get merged in before I give this one the green light to make sure everything is in sync - but consider this approved. Thank you both again so much! |
Yaaaay! 🚀 |
The BE fix has been merged! |
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.
Thank you @nichelle-hall! Good to go!
…to nasaownsky/1491-race-breakdown-other-textbox
Description of the change
Added "Other" textbox to race breakdown
Type of change
Related issues
closes #1491
Checklists
Development
This box MUST be checked by the submitter prior to merging:
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: