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

Fix Counter, Leaderboard and TimeSeries bugs #73

Merged
merged 7 commits into from
Jun 15, 2023

Conversation

felipecadavid
Copy link
Contributor

@felipecadavid felipecadavid commented Apr 21, 2023

This PR fixes some bugs found while testing the components in the example apps.

TimeSeries

  • Connected mode re-fetched data on styles change
  • Connected mode re-fetching on every re-render due to useEffect object dependencies

Leaderboard

  • Connected mode re-fetched data on styles change
  • Connected mode re-fetching on every re-render due to useEffect object dependencies
  • Switching from variant bar to table and then to bar again made bar chart disappear

Counter

  • Connected mode re-fetched data on styles change
  • Connected mode re-fetching on every re-render due to useEffect object dependencies
  • When Connected mode receiving a null value, it showed an error, changed to - instead
  • Sometimes Counter attempted to fetch data on Static mode, added !isStatic condition to fix

@vercel
Copy link

vercel bot commented Apr 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
ui-kit ⬜️ Ignored (Inspect) Visit Preview Jun 13, 2023 5:10pm

Copy link
Contributor

@jonatassales jonatassales left a comment

Choose a reason for hiding this comment

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

Looking good. I think we could improve some parts

packages/react/counter/src/Counter.tsx Outdated Show resolved Hide resolved
packages/react/counter/src/Counter.tsx Outdated Show resolved Hide resolved
packages/react/time-series/src/TimeSeries.tsx Show resolved Hide resolved
Copy link
Contributor

@markandrus markandrus left a comment

Choose a reason for hiding this comment

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

@felipecadavid +1 for the detailed PR description. Let's chat tomorrow about

  1. How we document feature additions/bug fixes in this project.
    • We might want a pull request template.
    • We might start tracking a CHANGELOG.md in this project.
  2. How we manually validate (with @davepropel) that a PR like this is good-to-go.

packages/react/counter/src/Counter.tsx Outdated Show resolved Hide resolved
packages/react/counter/src/Counter.tsx Outdated Show resolved Hide resolved
}
if (typeof fetchedValue === 'undefined') {
setHasError(true)
console.error(`QueryError: Your metric ${query?.metric} returned undefined.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

@felipecadavid I am think we should probably remove the console.error for now, and plan for logging as a feature soon.

What I mean is that,

  1. First of all, customers should be able to enable/disable logging; and
  2. Second of all, customers may even want to provide their own callbacks to avoid sending directly to console.

Since we don't have that prepared yet, let's not add more non-configurable console.error, console.log, etc., calls to the library just yet.

Suggested change
console.error(`QueryError: Your metric ${query?.metric} returned undefined.`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to comment all the console.error and will leave a comment related to this

packages/react/counter/src/utils.ts Outdated Show resolved Hide resolved
Comment on lines +325 to +327
if (variant === 'table') {
destroyChart()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@felipecadavid Why are we only calling destroyChart for variant === 'table'? What about the other variant(s)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently only have 'table' and 'bar' variants for leaderboard, what I'm doing here is stating that when the variant changes to 'table' we should then cleanup the 'bar' chart, so we allow changing the component's variant dynamically

Copy link
Contributor

@markandrus markandrus left a comment

Choose a reason for hiding this comment

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

+1 — let's get this in a release candidate

@felipecadavid felipecadavid merged commit 2751f40 into main Jun 15, 2023
3 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.

None yet

3 participants