-
Notifications
You must be signed in to change notification settings - Fork 3
fix: editor component migration into main app #2833
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
Conversation
OpenAPI ChangesShow/hide 14 changes: 1 error, 0 warning, 13 infoUnexpected changes? Ensure your branch is up-to-date with |
c90be7a to
ace5d82
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.
I haven't looked at all the changes here yet, but wanted to call out a different approach for the API change.
Additionally, could you resolve the conflicts?
learning_resources/views.py
Outdated
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.
Request: Rather than adding a new action, let's just add to the existing LearningResourceFilter filterset—that's what controls the available query param filters on the view. Adding this
id = NumberInFilter(
label="A unique numeric identifier for the resources",
field_name="id",
)should get exactly the behavior we want.
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 have done this change you can review it now.
8ae0792 to
f96a1e3
Compare
ChristopherChudzicki
left a comment
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.
One small request regarding the naming of the query param: just id, not ids. That matches how we do other params (e.g., readable_id) and reads better for "exploded" params, ?id=1&id=2&id=3, whereas right now it would be ?ids=1&ids=2&ids=3....
OpenAPI Diff Failure: I would suggest we ignore this, though it would have been better to not merge #2832 (or make the change requested here in that PR prior to merging. Lots of PRs open!).
What are the relevant tickets?
https://github.com/mitodl/hq/issues/9724
Description (What does it do?)
The plan is to move the tiptap editor code from
ol-componentsto main app. The purpose is to consume the code which exists in main app as reusable code like drawer code and userList code. The decision was taken after having discussion with @ChristopherChudzickiScreenshots (if appropriate):
How can this be tested?
We just need to verify all the implemented features working as per implementation
Additional Context