feat!: use explicit stats' query params #59
Conversation
| var showProblems = "true" === "${problems}"; | ||
| var showLearners = "true" === "${learners}"; | ||
| var showInstructors = "true" === "${instructors}"; | ||
| var showVideos = "false" === "${hideVideos}"; |
There was a problem hiding this comment.
I like the refactoring of the query parameters in the theme. The only thing is the logic, so In control, we have to thing in hidden items. So I have to use a blacklist logic.
Mainly, I am looking at this view as the feedback API endpoint for example. We have a boolean parameter public.
If we set it true it means public but if I set it false not public or "private, hide-public". The thing is that we want to manage this logic of blacklist?
Also, I have a mismatch of the query param, this is a django view, query params in snake case... but ups the react component use Camelcase.
There was a problem hiding this comment.
please stop using racist terms, but anyway that is just a semantic thing if the label say hide or show the user should be smart enough to select the right option
| var showVideos = "false" === "${hideVideos}"; | ||
| var showCourses = "false" === "${hideCourses}"; | ||
| var showProblems = "false" === "${hideProblems}"; | ||
| var showLearners = "false" === "${hideLearners}"; |
There was a problem hiding this comment.
Here is that thing where we have to consider changing the names and matching connection with component, why if the component has ShowCourses we have to connect it with hideCourses? This could be confusing.
😶🌫️
There was a problem hiding this comment.
this comment is weird since you removed the thing that you are proposing
There was a problem hiding this comment.
Actually, I changed it to true in my first approach. Then due to my confusion about snake_case and camel_case, I changed to videos, courses, problems.learners.. they all fit snake and camel case
There was a problem hiding this comment.
yes, I checked again your changes and you didn't proposed any change in the way how the api is consumed, you only changed the default values to show everything but using confusing names
|
closed in favor of #60 |
Description
This implements explicit stats' query params since the current options are ambiguous
Testing instructions
/eox-nelp/frontend/stats/tenant/?hideVideos=true&hideCourses&hideLearners=falseResult