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

Refactor state management to useContext and useReducer #138

Merged
merged 18 commits into from
Nov 13, 2023

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 1, 2023

Telescoping on #141
Fixes AP-3815

Big refactor to get rid of a lot of prop drilling in favor of useContext. For the selectedBuild behavior I used a reducer.

I wouldn't say this is the final solution but it's making a step in the right direction.

📦 Published PR as canary version: 0.0.116--canary.138.b91410d.0

✨ Test out this PR locally via:

npm install @chromaui/addon-visual-tests@0.0.116--canary.138.b91410d.0
# or 
yarn add @chromaui/addon-visual-tests@0.0.116--canary.138.b91410d.0

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

If you think this is better, then sure. I'm a bit worried because it was tricky to get all that selected build stuff right, and you've moved the logic for when we calculate it. But I guess if all the stories pass I'm OK with it.

I would suggest this is a PR that should get merged with 0 story changes overall.

src/screens/VisualTests/VisualTests.tsx Outdated Show resolved Hide resolved
src/screens/VisualTests/VisualTests.tsx Outdated Show resolved Hide resolved
src/utils/action.ts Show resolved Hide resolved
src/screens/VisualTests/VisualTests.tsx Outdated Show resolved Hide resolved
@ghengeveld ghengeveld changed the base branch from main to fix-console-warnings November 3, 2023 15:42
Base automatically changed from fix-console-warnings to main November 3, 2023 22:11
Copy link
Contributor

@weeksling weeksling left a comment

Choose a reason for hiding this comment

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

I have some more review to do and I'll try to look more closely sometime this weekend.

But for now, I noticed this same bug in this pr. It seems like it resets the controls whenever the the polling reruns #139 (review)

As well, I think we'll need the same context inside the onboarding and maybe other screens one day, so maybe we could add the ControlsContext and BuildContext to a separate directory, like src/state/ so we can easily find them and use them anywhere in the application if needed?

@ghengeveld
Copy link
Member Author

I noticed this same bug in this pr. It seems like it resets the controls whenever the the polling reruns #139 (review)

Yes I fixed that on this PR just now. I'll update the other PR to have the latest from this one.

As well, I think we'll need the same context inside the onboarding and maybe other screens one day, so maybe we could add the ControlsContext and BuildContext to a separate directory, like src/state/ so we can easily find them and use them anywhere in the application if needed?

Yeah I've been thinking about centralizing state in its own directory, though I'm not certain about it because in general I prefer to colocate and group by module/feature rather than by technical similarity. Controls should actually live higher up anyway I suppose (because it contains the warnings and settings toggles). For now I'll keep it as-is. We'll undoubtedly revisit it later when we build out more screens.

@ghengeveld ghengeveld merged commit b1bd90f into main Nov 13, 2023
6 checks passed
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.

3 participants