Skip to content

refactor: Dashboard context#1249

Open
MoranGoz wants to merge 6 commits intomainfrom
refactor-dashboardContext
Open

refactor: Dashboard context#1249
MoranGoz wants to merge 6 commits intomainfrom
refactor-dashboardContext

Conversation

@MoranGoz
Copy link
Copy Markdown
Collaborator

Description

screenshots

@MoranGoz MoranGoz requested a review from AvivAbachi as a code owner August 23, 2025 14:05
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 need fix the stories so it build

  • DayTimeChart.stories.tsx
  • TimeChart.stories.tsx
  • WorstLinesChart.stories.tsx

if you need help, feel free to ask me

export const DashboardCtx = createContext<DashboardContextType>({} as DashboardContextType)
export const DashboardContext: FC<PropsWithChildren> = ({ children }) => {
// loading
const [allLinesChartsIsLoading, setAllLinesChartsIsLoading] = useState(false)
Copy link
Copy Markdown
Collaborator

@AvivAbachi AvivAbachi Aug 23, 2025

Choose a reason for hiding this comment

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

maybe we can simplify state?

  const [allLinesChartsIsLoading, setAllLinesChartsIsLoading] = useState({
    loading: false,
    empty: false,
  })

or

type ErrorItem = false | 'loading' | 'error'
const [worstLineIsLoading, setWorstLineIsLoading] = useState<ErrorItem>(false)

or

type ErrorItem = { name: string; state: false | 'loading' | 'error' }
const [errorList, setErrorList] = useState<ErrorItem[]>([])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First - I'd like to emphasise that I think the code is great as is - you've done a great job.
following @AvivAbachi's suggestions, here's some ideas that I have. It's up to you - you're the owner :)

First, if we'll use useQuery, it can make our life easier using the useIsFetching:

https://tanstack.com/query/latest/docs/framework/react/reference/useIsFetching

Second idea - you can use one variable for the state. I disagree with Aviv regarding the idea to use array - I believe arrays should be used when ordering matters, and I don't feel like that's the case. Here's my suggestion draft -

  const [statuses, setStatuses] = useState<
      'worstLiteChart' | 'otherChart' | 'theThirdChart' | ..., // so on
     'loading' | 'loaded-empty' | 'loaded' | 'error'
>({})

Third option - having different state variables - it's also great and valid - just like you've implemented.
It's up to you, anyway that's a great change 👑

const [isDataEmpty, setIsDataEmpty] = useState(false)
const [isDataLoading, setIsDataLoading] = useState(false)

useEffect(() => {
Copy link
Copy Markdown
Collaborator

@AvivAbachi AvivAbachi Aug 23, 2025

Choose a reason for hiding this comment

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

you can useMemo() is same computed() in vue

  const isDataEmpty = useMemo(() => {
    return allLineChartsIsEmpty && worstLineIsEmpty && dayTimeChartIsEmpty
  }, [allLineChartsIsEmpty, worstLineIsEmpty, dayTimeChartIsEmpty])

@AvivAbachi AvivAbachi changed the title Refactor dashboard context refactor: Dashboard context Aug 23, 2025
Comment on lines +43 to +44
isDataLoading,
isDataEmpty,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When someone consumes this context, I don't want them to make guesses on whether there's an "and" (&&) or "or" (||) condition between the different statuses. I suggest that we could try a naming convention that corresponds with the Array.prototype.every() and Array.prototype.some()

Suggested change
isDataLoading,
isDataEmpty,
isSomeDataLoading,
isAllDataEmpty,

export const DashboardCtx = createContext<DashboardContextType>({} as DashboardContextType)
export const DashboardContext: FC<PropsWithChildren> = ({ children }) => {
// loading
const [allLinesChartsIsLoading, setAllLinesChartsIsLoading] = useState(false)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First - I'd like to emphasise that I think the code is great as is - you've done a great job.
following @AvivAbachi's suggestions, here's some ideas that I have. It's up to you - you're the owner :)

First, if we'll use useQuery, it can make our life easier using the useIsFetching:

https://tanstack.com/query/latest/docs/framework/react/reference/useIsFetching

Second idea - you can use one variable for the state. I disagree with Aviv regarding the idea to use array - I believe arrays should be used when ordering matters, and I don't feel like that's the case. Here's my suggestion draft -

  const [statuses, setStatuses] = useState<
      'worstLiteChart' | 'otherChart' | 'theThirdChart' | ..., // so on
     'loading' | 'loaded-empty' | 'loaded' | 'error'
>({})

Third option - having different state variables - it's also great and valid - just like you've implemented.
It's up to you, anyway that's a great change 👑

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