Skip to content

feat: insert error context#1186

Open
krasnoff wants to merge 16 commits intomainfrom
kk-insert-context
Open

feat: insert error context#1186
krasnoff wants to merge 16 commits intomainfrom
kk-insert-context

Conversation

@krasnoff
Copy link
Copy Markdown
Collaborator

@krasnoff krasnoff commented Jul 5, 2025

Description

In the dashboard screen I inserted an error context instead using props

@krasnoff krasnoff requested a review from NoamGaash as a code owner July 5, 2025 06:07
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 5, 2025

@NoamGaash
Copy link
Copy Markdown
Member

Hi @krasnoff !
I'm not sure that ErrorContext is the right thing here - because we don't want to throw exception, it's just a warning that shouldn't block anything.
Consider using Reacts useContext.
In your preview, seems like trying to use the "חברה מפעילה" page yields an error page (you can also see that in the tests)

@krasnoff krasnoff requested a review from AvivAbachi as a code owner July 12, 2025 06:24
@krasnoff
Copy link
Copy Markdown
Collaborator Author

I fixed the bug in "חברה מפעילה" page

Copy link
Copy Markdown
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

It looks great, except for the naming conventions
There's a documentation regarding "Assigning to new variable names" that you may find helpful in this context, let me know if you need any help / assistance. That's a great work.

@NoamGaash
Copy link
Copy Markdown
Member

oh, and when thinking about it - maybe we want to consider calling that WarningContext instead?

@AvivAbachi
Copy link
Copy Markdown
Collaborator

@krasnoff I updated your PR form main; it should help with the random test failing.
I also updated the dependency, so you need to use npm install.

Copy link
Copy Markdown
Collaborator

@AvivAbachi AvivAbachi left a comment

Choose a reason for hiding this comment

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

you can add <WarningContextProvider> to ThemeContext.tsx so it be work on all components and is work on storybook,
now is crash the test.

@NoamGaash
Copy link
Copy Markdown
Member

Hi @krasnoff , feel free to re-open this PR once you're available to continue :)

@NoamGaash NoamGaash closed this Jan 26, 2026
@krasnoff
Copy link
Copy Markdown
Collaborator Author

krasnoff commented Feb 1, 2026

Let me get this straight:
In page DashboardPage.tsx The child componenets: AllLinesChart, WorstLinesChart and DayTimeChart notifies the parent component through props.
Do you want me to create a custom <WarningContextProvider> and to use it instead of the props?

@NoamGaash NoamGaash reopened this Feb 2, 2026
@NoamGaash
Copy link
Copy Markdown
Member

NoamGaash commented Feb 2, 2026

Hi @krasnoff !
There are few things that can be done to make this PR reviewable and then mergeable -

  • fix the merge conflicts - I can see that some HAR files were changed by you, I'm not sure why - it would probably be for the best to align it with the main branch version
  • address the previous review comments regarding naming conventions (setValue, for example)

Thanks!

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