Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Taxonomy export menu [FC-0036] #645
feat: Taxonomy export menu [FC-0036] #645
Changes from 7 commits
4963124
4077397
e87c545
4922420
5e12a8f
09aa8b6
1925fae
0be804c
da0156f
0cc473c
5af8d66
a21fac2
61864d3
b09b86a
df8432d
1abf034
1d6f9a5
5c4c87a
7e514b6
080b03e
16233fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
Check warning on line 19 in src/taxonomy/api/hooks/api.js
Codecov / codecov/patch
src/taxonomy/api/hooks/api.js#L19
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 don't think selectors is the correct place for this, although I think this might be a preexisting problem with this module.
I think it's important to keep this ADR in mind: https://github.com/openedx/frontend-template-application/blob/master/docs/decisions/0002-feature-based-application-organization.rst
None of the functions here are hooks or selectors in the commonly understood context of those terms.
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.
@xitij2000 I updated the UI components to match with the ADR here: b09b86a
About the data layer, we have try to use useQuery to get data. On df8432d I tried to match with the structure on the ADR, @xitij2000 could you check it?
CC @bradenmacdonald
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.
Given that this is using useQuery instead of redux I'm not sure how things would map. However, I think it's best to avoid confusion with exist terms like hooks, thunks, selectors, if you are not using those since someone else working on this might have difficulty navigating this codebase in that case.
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.
Thanks! I understand. I deleted
thunks.js
and I movehooks.jsx
toapi/apiHooks.jsx
(to avoid use hooks or any other exist term). Also I added a docstring inapi/apiHooks.jsx
to explain all of this.