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

Make React frontend capable of reading API results under a results key #4159

Merged
merged 3 commits into from
May 31, 2024

Conversation

RobertJoonas
Copy link
Contributor

Changes

With the new imported data filtering feature, we'll start returning warnings in the dashboard API results, which means we'll need to move the actual list of results under a separate results key.

This PR is a preparation in order to not break the frontend when suddenly receiving API results under a results key. For some time, we'll need to be able to handle both a straight-up list response, or a map, containing the results key.

Tests

  • This PR does not require tests

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • This PR does not change the UI

@RobertJoonas RobertJoonas added deploy-to-staging Special label that triggers a deploy to a staging environment preview labels May 29, 2024
Comment on lines -14 to 15
return api.get(apiPath(site, '/countries'), query, {limit: 9}).then((res) => {
return res.map(row => Object.assign({}, row, {percentage: undefined}))
})
return api.get(apiPath(site, '/countries'), query, { limit: 9 })
}
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 believe this manual mapping to percentage undefined was used in order to prevent displaying percentages in this view:

image

However, the ListReport component does not render it anyway unless it's specified in the metrics prop, so the outcome is essentially the same without this manual mapping.

@RobertJoonas RobertJoonas requested a review from zoldar May 29, 2024 15:29
add length key back

Co-authored-by: Adrian Gruntkowski <[email protected]>
Copy link
Contributor

@ukutaht ukutaht left a comment

Choose a reason for hiding this comment

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

Is there a plan to clean up the fallbacks once backend changes are done as well? I'm asking because IMO it doesn't make sense to keep accepting multiple different response formats when we know for sure what is being returned from internal APIs.

@RobertJoonas
Copy link
Contributor Author

Is there a plan to clean up the fallbacks once backend changes are done as well? I'm asking because IMO it doesn't make sense to keep accepting multiple different response formats when we know for sure what is being returned from internal APIs.

Yes, @ukutaht. The PR that implements filtering will remove it so we'll start relying only on the results key.

@RobertJoonas RobertJoonas merged commit 57e28f0 into master May 31, 2024
10 checks passed
@RobertJoonas RobertJoonas deleted the dashboard-api-results-key branch May 31, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy-to-staging Special label that triggers a deploy to a staging environment preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants