-
Notifications
You must be signed in to change notification settings - Fork 81
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: collections page [FC-0062] #1281
Merged
bradenmacdonald
merged 22 commits into
openedx:master
from
open-craft:navin/FAL-3790-collections-page
Sep 20, 2024
+1,244
−103
Merged
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
cdb633b
feat: collections bare bones page
navinkarkera 0f755d9
feat: hook to fetch single meilisearch document
navinkarkera 3f6a32e
feat: collection sidebar
navinkarkera ac6709d
feat: components in collections
navinkarkera bfd3e7d
fix: rebase issues
navinkarkera 5b2d734
fix: darken card action btn hover state
navinkarkera 6841d9d
refactor: pass btn click handler to empty state component
navinkarkera 626c06b
refactor: move messages related to collections in single file
navinkarkera 1b3754c
fix: lint issues
navinkarkera fd1aa85
test: add test for collections page
navinkarkera 347871b
fix: lint issues
navinkarkera 0f4bad3
fix: collection querykey
navinkarkera c8476d1
test: ignore unused functions
navinkarkera d510554
refactor: remove unnecessary comment
navinkarkera f9b2e52
feat: create component under collection
navinkarkera 5846b83
fix: test
navinkarkera 2ae6c98
fix: lint issues
navinkarkera 26c0a9e
feat: open collections page on save
navinkarkera 2c06446
refactor: update collection url
navinkarkera 3395c1e
fix: lint issues
navinkarkera 2848746
refactor: fetch single collection info from meilisearch
navinkarkera d388335
feat: api call for adding component to collection
navinkarkera File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
fix: lint issues
commit 1b3754c0faa8ff5b5556656e6cf48d5887599ed7
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,5 +23,5 @@ | |
|
||
// Reduce breadcrumb bottom margin | ||
ol.list-inline { | ||
margin-bottom: 0rem; | ||
margin-bottom: 0; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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 quite understand why we need this flag if we have to add
useCollection
to fetch the collection anyway?I would have thought that thoseextraFilter
s you're sending toSearchContextProvider
would ensure that the first result in the collection query (multisearch number 3) is the Collection we want to show here.Ah sorry.. the second filter is
collections.key=collectionId
, and for this to work we'd needblock_id = collectionId
.. I guess it could be an "OR" statement, but I'm not sure that complexity is worth it here.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.
@pomegranited
useCollection
gets collection data from API while this flag controls fetching of all collections based on search. We only want to fetch blocks under this collection which is why we have update the extraFilter. As I mentioned in the comment above, there seems to be some issue with fetching single document from meilisearch.But I do like the idea of using block_id field which is currently set to
collection.key
but addingcontent_key
i.e. library.key into the filter we can get an individual document. If you agree, I can update it and remove the need to calling collections api to fetch single collection data.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.
Oh yes please -- it would be better if we could use meilisearch's data instead of hitting the backend, since we're doing that everywhere else.
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.
@pomegranited Done. Had to include
block_id
to filterable attributes in edx-platform and update search manager to override queries.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.
Noted, and thanks for making that change @navinkarkera . We just need to remember to run
reindex_search
on the tagging sandbox when we deploy this change for testing.