-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adds word cloud xblock extracting from edx-platform repo #4
Conversation
200a788
to
91432f2
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.
Just a couple nits so far, but generally this is looking great. I still need to test it myself, but it's exciting to hear that you have this working in edx-platform 🔥
I'm going to hold off on testing this for now. Let's focus on getting all the stub XBlocks into #2, getting that reviewed and merged, and then getting the waffle flags into edx-platform. That way, it will be easy to test this PR in edx-platform. |
5ed4627
to
3c289df
Compare
b5b6bf9
to
24544fb
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.
Could you rebase this Farhan? Then I'll be happy to test this using new flag we've added to edx-platform.
@kdmccormick This PR's branch is already up-to-date with the
Please suggest if we have better ways to do |
@farhan to test locally with tutor, you can mount the repo and rebuild+reboot:
With that setup, you should be able to |
cb3e29e
to
2566773
Compare
201dee6
to
000dee1
Compare
f1e4d36
to
a43d3ea
Compare
Farhan mentioned in sync-up that this is ready for review. |
a43d3ea
to
ab17b6f
Compare
ab17b6f
to
e632fb3
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 gave this a skim, and I see nothing wrong architecturally 👍🏻
Please take a look at the testing issues I've mentioned in openedx/edx-platform#35983, and also please have someone on Aximprovements review this PR . Once both of those are done, I'm happy to give a quick final review pass.
@kdmccormick Thanks for looking into it I have fixed the issue and everything is working fine now |
@@ -20,7 +20,12 @@ def test_my_student_view(self): | |||
as_dict = frag.to_dict() | |||
content = as_dict["content"] | |||
self.assertIn( | |||
"WordCloudBlock: count is now", | |||
"Word cloud", |
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.
We should move all tests from https://github.com/openedx/edx-platform/blob/master/xmodule/tests/test_word_cloud.py to this file
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.
We can do it another PR. Let's priortize publishing of this XBlock 🚢
fdcab16
to
c46a60a
Compare
@kdmccormick Please review it as it's waiting for your thumbs up for the release. |
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.
LGTM
Adds word cloud xblock extracting from edx-platform repo
Test PR
Make a test PR on
edx-platform
like this to run test cases on under development PR.Some on going changes in word cloud xblock
https://github.com/openedx/edx-platform/pull/36199/files#diff-a0f8168df87bd0334ca63f0abf7c52b2be89224c80bf10aad43ba00234cb21d0
File references:
xblocks_contrib/word_cloud/word_cloud.py
has been extracted fromxblocks_contrib/word_cloud/static/html/word_cloud_xblock.html
has been extracted fromxblocks_contrib/word_cloud/static/js/src/word_cloud_xblock.js
and otherd3-cloud files
has been extracted fromxblocks_contrib/word_cloud/static/css/word_cloud_xblock.css
has been extracted fromlms/static/css/WordCloudBlockDisplay.css
Acceptance Criteria: