-
Notifications
You must be signed in to change notification settings - Fork 0
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
[temp] feat: add import taxonomy button #8
[temp] feat: add import taxonomy button #8
Conversation
…x#647) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
openedx#650) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…x#641) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…openedx#651) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…v1.175.1 (openedx#663) * fix(deps): update dependency @edx/frontend-lib-content-components to v1.175.1 * fix: upgrade paragon --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: KristinAoki <[email protected]>
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.
src/taxonomy/data/thunks.js
Outdated
const getTaxonomyName = () => { | ||
let taxonomyName = null; | ||
while (!taxonomyName) { | ||
taxonomyName = prompt('Enter a name for the new taxonomy'); |
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.
Strings should be translatable.
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.
And using prompt
and alert
is temporary -- please add a comment at the top of this function to this effect so upstream know why we're doing it this way for now.
acf683e
to
da5e3f7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## chris/FAL-3528-taxonomy-export-menu #8 +/- ##
======================================================================
Coverage ? 87.52%
======================================================================
Files ? 424
Lines ? 6695
Branches ? 1437
======================================================================
Hits ? 5860
Misses ? 804
Partials ? 31 ☔ View full report in Codecov by Sentry. |
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 looks and works great @rpenido !
I'm not sure if we need tests added though? It might be difficult to test this temporary prompt
/alert
flow.
- I tested this on my devstack with this branch merged with feat: bare bones taxonomy detail page [FC-0036] openedx/frontend-app-authoring#655 so I could see the imported tags.
- I read through the code
- I checked for accessibility issues by using my keyboard to navigate.
- Includes documentation
- All strings are translatable.
da5e3f7
to
0c60c37
Compare
I agree. We expect this code to be temporary. I will check if the coverage report would allow us to let it pass. Closed in favor of: |
No description provided.