-
Notifications
You must be signed in to change notification settings - Fork 1
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: add tenant stats view #57
Conversation
2a84ecc
to
41b1cf9
Compare
This PR exceeds the recommended size of 1000 lines. Check if you are NOT addressing multiple issues with one PR. If is not the case continue the review process. |
eox_nelp/urls.py
Outdated
@@ -29,4 +29,5 @@ | |||
include('eox_nelp.course_experience.frontend.urls', namespace='course-experience-frontend'), | |||
), | |||
path('api/stats/', include('eox_nelp.stats.api.urls', namespace='stats-api')), | |||
path('stats/', include('eox_nelp.stats.urls', namespace='stats')), |
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.
Here we have to discuss which would be the default path. I like the prefix frontend because as a user I relate that it has react or some js extra.
https://github.com/eduNEXT/eox-nelp/pull/57/files#diff-99d9cbe815961e1cc232cd5191394ac2e794e5ea823be05f5f9addf7331a39a7R28
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.
That's right the react components should be in the frontend and that's all, if I didn't include the view, inside the frontend folder, is because a Django view is not a frontend element and that logic is not js or css and finally to add the path frontend in the url is redundant a view is an API or belongs to the presentation layer and you probably won't find the path frontend in other views
chore: update essentials version chore: update styles to remove background
0a9f0cd
to
81d0991
Compare
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.
I approve. I made a change but in another commit if necessary for future management. I gonna merge it.
By default the view show all components. Then you can manage or filter which component wouldnt be show setting it to false. feat: use frontend as experience use it This would be considered in future of the need of a refactor, but by the moment is better to have the templates views that use react with the same shape. chore: apply suggestions from code review
81d0991
to
d9090a8
Compare
Description
Add stats view issue https://edunext.atlassian.net/browse/FUTUREX-443