-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: GTFS Schedule Analytics UI #653
Conversation
Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-653-dtxmp9ro.web.app |
First things first: WOW! This is truly incredible @cka-y 🥳🥳🥳 Small feedback on my end after looking at the analytics UI:
|
@isabelle-dr thank you for the feedback! Let's address each of your points:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general comment, as we are adding a new screen in this PR, I suggest adding the labels/text to the internationalization files.
Thank you for clarifying! I ended up figuring that this was how to find the number, but still, it wasn't super clear at first glance if this was the number of pages or feeds, and I ended-up looking for a csv export so I could count and be 100% sure 🙈 |
Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-653-rh3m6ztt.web.app |
), | ||
}); | ||
|
||
// TODO improve this code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be addressed as part of a separate issue
return <div>Loading...</div>; | ||
} | ||
|
||
// TODO improve this code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as well
I'm ok with this PR; I'll let @Alessandro100 give the final approval |
web-app/src/app/screens/Analytics/GBFSFeedAnalytics/GBFSFeedAnalyticsTable.tsx
Outdated
Show resolved
Hide resolved
import { getLocationName } from '../../services/feeds/utils'; | ||
import { getAnalyticsBucketEndpoint } from '../../screens/Analytics/GTFSFeedAnalytics'; | ||
|
||
function* fetchFeedMetricsSaga( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[small optimization] Since both calls are independent of each other
const [response1, response2] = yield all([
...
])
would speed up the endpoint. As both of these calls are quite quick, it's not a show stopper or can be done in another ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very very impressive, it works well and is very insightful. I left small comments but nothing show stopping. I learned about material-react-table
and it's incredibly powerful. Overall this is really cool to have ✅
Thanks for making all my dreams come true @cka-y 🙏 And @davidgamez and @Alessandro100 for these thorough reviews! (I've been stalking the back and forth) |
Summary:
Closes #651 by implementing a user-friendly UI for the GTFS schedule feeds analytics project. The changes include:
This UI is designed to provide insights into the data’s quality, distribution, and changes over time, making GTFS schedule feeds data more accessible and actionable.
Expected behavior:
Upon deployment and testing of this PR, the UI should:
Testing tips:
Metrics
view is currently disabled to the publicTesters are invited to follow the tips AND to try anything they deem relevant outside the bounds of the testing tips.
Please make sure these boxes are checked before submitting your pull request - thanks!
./scripts/api-tests.sh
to make sure you didn't break anything