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.

Sign up for GitHub

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

Jump to bottom

Fix Counter, Leaderboard and TimeSeries bugs #73

Merged
merged 7 commits into from
Jun 15, 2023
Merged

Fix Counter, Leaderboard and TimeSeries bugs #73

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9e5c7ff
fix: fixes to TimeSeries component
felipecadavid Apr 21, 2023
1489391
fix: Leaderboard fixes
felipecadavid Apr 21, 2023
3f4ad32
fix: counter fixes
felipecadavid Apr 21, 2023
ac9f929
fix: static mode bugs
felipecadavid Apr 21, 2023
3ba2c22
refactor: remove unnecessary import
felipecadavid Apr 21, 2023
b5febb9
refactor: use ?? + remove unnecessary typeof
felipecadavid Jun 13, 2023
9254f09
refactor: comment all console.error
felipecadavid Jun 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions packages/react/counter/src/Counter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export function Counter(props: CounterProps) {
const [hasError, setHasError] = React.useState(false)
const [isLoading, setIsLoading] = React.useState(false)

const filtersString = JSON.stringify(query?.filters || [])

/**
* Fetches the counter data
* when the user doesn't provide
Expand All @@ -64,14 +66,21 @@ export function Counter(props: CounterProps) {
try {
setIsLoading(true)

const filters = JSON.parse(filtersString)

const response = await request(
PROPEL_GRAPHQL_API_ENDPOINT,
CounterDocument,
{
uniqueName: query?.metric,
counterInput: {
timeRange: query?.timeRange,
filters: query?.filters,
timeRange: {
relative: query?.timeRange?.relative || null,
n: query?.timeRange?.n || null,
start: query?.timeRange?.start || null,
stop: query?.timeRange?.stop || null
markandrus marked this conversation as resolved.
Show resolved Hide resolved
},
filters,
propeller: query?.propeller
}
},
Expand All @@ -90,7 +99,16 @@ export function Counter(props: CounterProps) {
} finally {
setIsLoading(false)
}
}, [query])
}, [
query?.metric,
query?.accessToken,
query?.timeRange?.n,
query?.timeRange?.relative,
query?.timeRange?.start,
query?.timeRange?.stop,
query?.propeller,
filtersString
])

React.useEffect(() => {
function handlePropsMismatch() {
Expand Down Expand Up @@ -118,18 +136,19 @@ export function Counter(props: CounterProps) {
async function setup() {
if (isStatic && value) {
setDataValue(value)
return
}

const fetchedValue = await fetchData()
if (!isStatic) {
const fetchedValue = await fetchData()

if (!fetchedValue) {
setHasError(true)
console.error(`QueryError: Your metric ${query?.metric} returned undefined or a \`null\` value.`)
return
}
if (typeof fetchedValue === 'undefined') {
markandrus marked this conversation as resolved.
Show resolved Hide resolved
markandrus marked this conversation as resolved.
Show resolved Hide resolved
setHasError(true)
console.error(`QueryError: Your metric ${query?.metric} returned undefined.`)
markandrus marked this conversation as resolved.
Show resolved Hide resolved
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

return
}

setDataValue(fetchedValue)
setDataValue(fetchedValue)
}
}

setup()
Expand Down
6 changes: 5 additions & 1 deletion packages/react/counter/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ const getValue = (options: getValueOptions) => {
const { value, localize } = options

const isInteger = Number.isInteger(parseFloat(value))
const isNull = value === null

if (isInteger) {
return localize ? parseInt(value).toLocaleString() : parseInt(value)
}
if (isNull) {
return '-'
}

return localize
? parseFloat(parseFloat(value).toFixed(2).toLocaleString()).toLocaleString()
Expand All @@ -25,7 +29,7 @@ export const getValueWithPrefixAndSufix = (params: {
}) => {
const { prefix, value, sufix, localize } = params

if (!value) return
if (typeof value === 'undefined') return
markandrus marked this conversation as resolved.
Show resolved Hide resolved

return (prefix ? prefix : '') + getValue({ value, localize }) + (sufix ? sufix : '')
}
44 changes: 27 additions & 17 deletions packages/react/leaderboard/src/Leaderboard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ export function Leaderboard(props: LeaderboardProps) {
const idRef = React.useRef(idCounter++)
const id = `leaderboard-${idRef.current}`

const filtersString = JSON.stringify(query?.filters || [])
const dimensionsString = JSON.stringify(query?.dimensions || [])

/**
* The html node where the chart will render
*/
Expand Down Expand Up @@ -196,18 +199,26 @@ export function Leaderboard(props: LeaderboardProps) {
try {
setIsLoading(true)

const dimensions = JSON.parse(dimensionsString)
const filters = JSON.parse(filtersString)
markandrus marked this conversation as resolved.
Show resolved Hide resolved

const response = await request(
PROPEL_GRAPHQL_API_ENDPOINT,
LeaderboardDocument,
{
uniqueName: query?.metric,
leaderboardInput: {
timeRange: query?.timeRange,
filters: query?.filters,
filters,
propeller: query?.propeller,
sort: query?.sort,
rowLimit: query?.rowLimit,
dimensions: query?.dimensions
dimensions,
timeRange: {
relative: query?.timeRange?.relative || null,
n: query?.timeRange?.n || null,
start: query?.timeRange?.start || null,
stop: query?.timeRange?.stop || null
}
}
},
{
Expand All @@ -228,13 +239,16 @@ export function Leaderboard(props: LeaderboardProps) {
}
}, [
query?.accessToken,
query?.dimensions,
query?.filters,
dimensionsString,
filtersString,
query?.metric,
query?.propeller,
query?.rowLimit,
query?.sort,
query?.timeRange
query?.timeRange?.n,
query?.timeRange?.relative,
query?.timeRange?.start,
query?.timeRange?.stop
])

React.useEffect(() => {
Expand Down Expand Up @@ -275,17 +289,7 @@ export function Leaderboard(props: LeaderboardProps) {
if (!isStatic) {
fetchChartData()
}
}, [
isStatic,
query?.timeRange,
query?.filters,
query?.propeller,
query?.sort,
query?.rowLimit,
query?.dimensions,
query?.rowLimit,
fetchData
])
}, [isStatic, fetchData])

React.useEffect(() => {
if (isStatic) {
Expand Down Expand Up @@ -317,6 +321,12 @@ export function Leaderboard(props: LeaderboardProps) {
}
}, [])

React.useEffect(() => {
if (variant === 'table') {
destroyChart()
}
Comment on lines +325 to +327
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

}, [variant])

if (isLoading || loading) {
destroyChart()
return <Loader styles={styles} />
Expand Down
27 changes: 23 additions & 4 deletions packages/react/time-series/src/TimeSeries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ export function TimeSeries(props: TimeSeriesProps) {
const idRef = React.useRef(idCounter++)
const id = `time-series-${idRef.current}`

const filtersString = JSON.stringify(query?.filters || [])

/**
* The html node where the chart will render
*/
Expand Down Expand Up @@ -257,15 +259,22 @@ export function TimeSeries(props: TimeSeriesProps) {
try {
setIsLoading(true)

const filters = JSON.parse(filtersString)
markandrus marked this conversation as resolved.
Show resolved Hide resolved

const response = await request(
PROPEL_GRAPHQL_API_ENDPOINT,
TimeSeriesDocument,
{
uniqueName: query?.metric,
timeSeriesInput: {
timeRange: query?.timeRange,
timeRange: {
relative: query?.timeRange?.relative || null,
n: query?.timeRange?.n || null,
start: query?.timeRange?.start || null,
stop: query?.timeRange?.stop || null
},
granularity,
filters: query?.filters,
filters: filters,
propeller: query?.propeller
}
},
Expand All @@ -285,7 +294,17 @@ export function TimeSeries(props: TimeSeriesProps) {
} finally {
setIsLoading(false)
}
}, [granularity, query?.accessToken, query?.filters, query?.metric, query?.propeller, query?.timeRange])
}, [
granularity,
query?.accessToken,
filtersString,
query?.metric,
query?.propeller,
query?.timeRange?.n,
query?.timeRange?.relative,
query?.timeRange?.start,
query?.timeRange?.stop
])

React.useEffect(() => {
function handlePropsMismatch() {
Expand Down Expand Up @@ -323,7 +342,7 @@ export function TimeSeries(props: TimeSeriesProps) {
if (!isStatic) {
fetchChartData()
}
}, [isStatic, query?.timeRange, query?.filters, query?.propeller, query?.granularity, query?.accessToken, fetchData])
}, [isStatic, fetchData])

React.useEffect(() => {
if (isStatic) {
Expand Down