-
Notifications
You must be signed in to change notification settings - Fork 95
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
[FC-0036] Refined taxonomy details page #833
[FC-0036] Refined taxonomy details page #833
Conversation
Thanks for the pull request, @ChrisChV! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #833 +/- ##
=======================================
Coverage 89.28% 89.28%
=======================================
Files 551 551
Lines 9740 9747 +7
Branches 2101 2103 +2
=======================================
+ Hits 8696 8703 +7
Misses 996 996
Partials 48 48 ☔ View full report in Codecov by Sentry. |
d66f954
to
a325011
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.
👍 Looks great, great work @ChrisChV !
Just left a small suggestion below, but I also had a question.
When flipping through the pagination of a taxonomy, like the Flat Taxonomy, the page number at the bottom updates as expect, however the result number is always Showing 100 of 5000.
regardless of which page we are on. Is this intended behavior? As opposed to have it display something like Showing 101-200 of 5000
for example. I'm not sure if it is within the scope of this PR or requirements in general.
- I tested this: I followed the test instructions, confirmed that the table header is no longer there, confirmed that pagination footer only shows up when there are more than 100 taxonomies and that it works, and confirmed that the "expand all" and up/down arrows are correctly on the right side for the table.
- I read through the code
- I checked for accessibility issues
- Includes documentation
-
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-secure
repository.
src/taxonomy/tag-list/data/api.js
Outdated
const getTagListApiUrl = (taxonomyId, page, pageSize) => new URL( | ||
`api/content_tagging/v1/taxonomies/${taxonomyId}/tags/?page=${page + 1}&page_size=${pageSize}`, | ||
getApiBaseUrl(), | ||
).href; |
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.
Just a suggestion that I think might help avoid any issues that would be hard to spot when needing to update/add queryparams in the future.
const getTagListApiUrl = (taxonomyId, page, pageSize) => new URL( | |
`api/content_tagging/v1/taxonomies/${taxonomyId}/tags/?page=${page + 1}&page_size=${pageSize}`, | |
getApiBaseUrl(), | |
).href; | |
const getTagListApiUrl = (taxonomyId, page, pageSize) => { | |
const url = new URL(`api/content_tagging/v1/taxonomies/${taxonomyId}/tags/`, getApiBaseUrl()); | |
url.searchParams.append('page', page + 1); | |
url.searchParams.append('page_size', pageSize); | |
return url.href; | |
}; |
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 think is out of the scope of this ticket. I have tried it with |
Perhaps we are not using DataTable correctly? If you look at the code, it is supposed to show something like "101-200": https://github.com/openedx/paragon/blob/7e4a81f9f2350c54362baa852c75dc32c2c2694e/src/DataTable/RowStatus.jsx#L24 |
If it's a bug in Paragon it's out of scope, but if it's our error on this page, please fix it here. |
8d8763a
to
90f3b7d
Compare
90f3b7d
to
45980ba
Compare
@bradenmacdonald Issue fixed here 45980ba |
Great, thanks for figuring that out @ChrisChV :) |
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.
Just small bits of feedback about using flex instead of float and using the url paramater building approach @yusuf-musleh pointed out.
.tag-list-table { | ||
.tag-table-expand-row { | ||
float: right; | ||
} | ||
|
||
table tr:first-child > th:nth-child(2) > span { | ||
// Used to move "Expand all" button to the right. | ||
// Find the first <span> of the second <th> of the first <tr> of the <table>. | ||
// | ||
// The approach of the expand buttons cannot be applied here since the | ||
// table headers are rendered differently and at the component level | ||
// there is no control of this style. | ||
float: right; | ||
} | ||
} |
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 think instead of this you should be able to use d-flex justify-content-end
for the first one and use display:flex; justify-content: flex-end
for the second.
IMO using float can sometimes cause issues since it breaks the normal flow of the document.
Thanks @xitij2000! I think it's ready for merge |
2 similar comments
Thanks @xitij2000! I think it's ready for merge |
Thanks @xitij2000! I think it's ready for merge |
@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR fixes the issues listed in openedx/modular-learning#186
Screenshots
Testing instructions
1. The table header has been removed (so "Showing 7 of 7" no longer appears).
2. The table displays up to 100 parent tags at a time. Pagination is only required if there are more than 100 parent tags.
Flat Taxonomy
and verify that the table shows 100 parent tags and there is pagination footer.TwoLevelTaxonomy
and verify that there is not pagination footer.The "expand all" link and the up/down arrows have been moved to the far right of the table.
Support information