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 all 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
2 changes: 1 addition & 1 deletion packages/core/components/src/ErrorBoundary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class ErrorBoundary extends Component<Props, State> {
}

componentDidCatch(error: Error, errorInfo: React.ErrorInfo) {
console.error(error, errorInfo)
// console.error(error, errorInfo) we will set logs as a feature later
}

render() {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/release/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ async function main(): Promise<void> {
}
}
} catch (error) {
console.error(error)
// console.error(error) we will set logs as a feature later
process.exit(1)
}
}
Expand Down
49 changes: 34 additions & 15 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
},
filters,
propeller: query?.propeller
}
},
Expand All @@ -90,20 +99,29 @@ 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() {
if (isStatic && !value) {
console.error('InvalidPropsError: You must pass either `value` or `query` props')
// console.error('InvalidPropsError: You must pass either `value` or `query` props') we will set logs as a feature later
setHasError(true)
return
}

if (!isStatic && (!query?.accessToken || !query?.metric || !query?.timeRange)) {
console.error(
'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop'
)
// console.error(
// 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop'
// ) we will set logs as a feature later
setHasError(true)
return
}
Expand All @@ -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 (fetchedValue === undefined) {
setHasError(true)
// console.error(`QueryError: Your metric ${query?.metric} returned undefined.`) we will set logs as a feature later
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 (value === undefined) return

return (prefix ? prefix : '') + getValue({ value, localize }) + (sufix ? sufix : '')
}
56 changes: 33 additions & 23 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,35 +239,38 @@ 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(() => {
function handlePropsMismatch() {
if (isStatic && !headers && !rows) {
console.error('InvalidPropsError: You must pass either `headers` and `rows` or `query` props')
// console.error('InvalidPropsError: You must pass either `headers` and `rows` or `query` props') we will set logs as a feature later
setHasError(true)
return
}

if (isStatic && (!headers || !rows)) {
console.error('InvalidPropsError: When passing the data via props you must pass both `headers` and `rows`')
// console.error('InvalidPropsError: When passing the data via props you must pass both `headers` and `rows`') we will set logs as a feature later
setHasError(true)
return
}
if (
!isStatic &&
(!query.accessToken || !query.metric || !query.timeRange || !query.dimensions || !query.rowLimit)
) {
console.error(
'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric`, `dimensions`, `rowLimit` and `timeRange` in the `query` prop'
)
// console.error(
// 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric`, `dimensions`, `rowLimit` and `timeRange` in the `query` prop'
// ) we will set logs as a feature later
setHasError(true)
return
}
Expand All @@ -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 All @@ -302,7 +306,7 @@ export function Leaderboard(props: LeaderboardProps) {
React.useEffect(() => {
try {
if (variant !== 'bar' && variant !== 'table') {
console.error('InvalidPropsError: `variant` prop must be either `bar` or `table`')
// console.error('InvalidPropsError: `variant` prop must be either `bar` or `table`') we will set logs as a feature later
throw new Error('InvalidPropsError')
}
setHasError(false)
Expand All @@ -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
37 changes: 28 additions & 9 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,25 +294,35 @@ 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() {
if (isStatic && !labels && !values) {
console.error('InvalidPropsError: You must pass either `labels` and `values` or `query` props')
// console.error('InvalidPropsError: You must pass either `labels` and `values` or `query` props') we will set logs as a feature later
setHasError(true)
return
}

if (isStatic && (!labels || !values)) {
console.error('InvalidPropsError: When passing the data via props you must pass both `labels` and `values`')
// console.error('InvalidPropsError: When passing the data via props you must pass both `labels` and `values`') we will set logs as a feature later
setHasError(true)
return
}
if (!isStatic && (!query.accessToken || !query.metric || !query.timeRange)) {
console.error(
'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop'
)
// console.error(
// 'InvalidPropsError: When opting for fetching data you must pass at least `accessToken`, `metric` and `timeRange` in the `query` prop'
// ) we will set logs as a feature later
setHasError(true)
return
}
Expand All @@ -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