-
Notifications
You must be signed in to change notification settings - Fork 122
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
Refactor society-wide AI component #2326
Refactor society-wide AI component #2326
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dd6b075
to
1353989
Compare
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.
Reaching out offline for more specific guidance on what's useful in this PR, but included some general approaches/tactics I would try to move towards in our UI in general.
import { getParameterAtInstant } from "../../../api/parameters"; | ||
import { MarkdownFormatter } from "../../../layout/MarkdownFormatter"; | ||
import { countryApiCall } from "../../../api/call"; | ||
import { getImpactReps } from "./ImpactTypes"; | ||
import ErrorComponent from "../../../layout/ErrorComponent"; | ||
import { Segmented } from "antd"; | ||
|
||
export function AudienceSelector(props) { |
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.
Discussion, non-blocking I'm going to throw a bunch of stuff at the wall here with the intent of informing our new app development, not necessarily trying to change this.
This component (AudienceSelector) has a lot of responsibilities in one place.
- Defining a new(?) UI element consisting of a title + multi-select buttons.
- Defining inline style for that UI element
- (re)defining the valid values for audience as defined in the API & the appropriate default value.
- business logic around setting external state and clearing the display (I.e. setAudience and reset display)
Suggestion, not now Use the design system
I am familiar with cloudscape. but antd has one too which I'm assuming does all the same things.
A framework has a bunch of widgets, but the design system actually tells you how to use them properly.
It defines how a full application is built using standard layouts and conventions built out of those widgets along with lots of guidance on what to prioritize and why.
All those layouts and conventions are (should be) well defined, supported, tested, work across different browsers and devices, support internationalization, etc. etc. etc.
Effectively if you pick one and align your UI design to it, you should write little to no CSS and everything will more or less work.
ALSO the react components themselves will follow a common set of coherent conventions and best practices that provide a great template for how to structure/configure/test the components we write.
I think if we started with "how do multi-selects work in antd" and, assuming the design system doesn't already address our use case in a different way, build our widgets following their lead, all my big concerns would be already addressed.
Suggested action Are we already discussing design systems with our design team? The last project I worked on we dramatically sped up development after aligning on a design system with the design team and then just using it to implement the page.
Because the design system encodes so many conventions and best practices clearly, it really reduced the friction between them, us, and the actual coding of the UIX.
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 is an interesting point. We have not already discussed design systems with our design vendor, nor have we done so internally. I'm also not sure if they are far enough along in their own process yet for them to actively consider one/tie it into what they're doing. I know we have a key findings meeting with them next week on February 18, so perhaps that would be a good time to broach this topic. I also don't know if we've ever done enough research ourselves into design systems to definitively say what our preferences might be.
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.
at some point we discussed ant vs material with @nikhilwoodruff. just raised this in #app-redesign with citizen codex
} | ||
} | ||
|
||
async function fetchPrompt() { |
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.
discussion, non-blocking with the new API, I would expect all this to be replaced by a generated react-query client. (explicit building of the path, specifying the POST operation, checking for an OK response, parsing the result, etc.)
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.
Interesting, thank you for flagging
function resetAnalysis() { | ||
setAnalysisError(false); | ||
setAnalysis(""); // Reset analysis content | ||
setAnalysisLoading(false); | ||
} |
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.
discussion, nonblocking so I see a bunch of interactions between local events and global state in the various components (I.e. when X happens in widget Y I want to reset the display state).
As per my note above, there are a number of ways to simplify what we're doing here, but at least as currently written I think a reducer would actually simplify this.
Effectively you
- Model state in shared context and
- Model changes to state in response to events.
I could try to outline how that works here, but suggest reading the docs instead since it's a bit involved and they already do a good job.
TLDR:
- the display component generally just accepts a current state value as a parameter and takes a callback for when some user action happens (I.e. value & on change). How/if/why/when an event changes the state is defined elsewhere.
- The containing component
- gets the current state from what's called a context and passes it to the display component and
- maps the onChange event to a reducer event.
- It knows about context and the reducer, but it doesn't know or care how events map to changes in state
- The reducer, takes the current state + the event and produces the new state - it doesn't care who triggered the event and doesn't care who displays the state or how, it just understands how state changes in response to events.
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 completely agree. I actually opened an issue myself in August 2023 here to use a state library like Redux, though I know React's own internal reducer tooling has grown quite a bit since then.
I also think we follow two patterns that vastly increase state management complexity for little to no gain:
- We use URL parameters extensively, even at points in the user flow in which I feel they have no utility, but changes to these require syncing with state
- We use effect hooks in numerous spots that either a. don't require them or b. should be re-engineered into a clearer, potentially more linear user flow that would obviate them
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.
Based on feedback approving this without any changes with the intent to focus on these systematic issues in the new app.
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.
video looks great!
Description
Fixes #2321
Fixes #2325
Depends upon review and merging of PolicyEngine/policyengine-api#2136; will remain in draft until they are merged.
Changes
This PR consumes changes made in the linked API PR, altering how we fetch AI prompts and output. It also handles the API PR's transition away from streaming for responses already present in our database, refactors front-end visuals to improve UX, and adds better error handling to prevent endless spinners.
Screenshots
AwesomeScreenshot-1_29_2025.6_26_28PM.webm
Tests
None have yet been added