Skip to content
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: xblock aside #31

Closed
wants to merge 1 commit into from
Closed

Conversation

Ian2012
Copy link
Contributor

@Ian2012 Ian2012 commented Oct 20, 2023

Description

This PR is an exploration / base implementation on creating an FeedbackXBlockAside.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 20, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @Ian2012! 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

Comment on lines +434 to +437
#vote_aggregate = List(
# default=None, scope=Scope.user_state,
# help=_("A list of user votes")
#)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope user_state_summary is not compatible with the XBlockAside as the fields are not cached.

This is because the fields are behind a write through cache that is not prefetch correctly due to incorrect implementations in edx-platform. More context here: openedx/edx-platform#33554

Comment on lines +470 to +473
#self.runtime.publish(self,
# 'edx.feedbackxblock.likert_provided',
# {'old_vote': self.user_vote,
# 'new_vote': data['vote']})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be updated in the base xblock to try - except this

@Ian2012 Ian2012 requested a review from feanil October 20, 2023 19:48
@Ian2012
Copy link
Contributor Author

Ian2012 commented Oct 20, 2023

Hi @feanil. I was planning to create a new model to store the data of only the aside data by adding a new context variable in the json_handler and reading it if needed for staff users.

Another option would be to add support for XBlock Fields in XBlockAsides, but that seems to a be a large contribution

@openedx-webhooks
Copy link

@Ian2012 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@feanil
Copy link
Contributor

feanil commented Nov 3, 2023

@Ian2012 did you mean to close this?

@Ian2012
Copy link
Contributor Author

Ian2012 commented Nov 3, 2023

Yes, for now, this work will remain incomplete. However, I would like to know what are your thoughts on this.

@feanil
Copy link
Contributor

feanil commented Nov 8, 2023

I'm not opposed to having the feedback xblock be an aside, I think I was trying to understand how it would affect all the other blocks and was having trouble visualizing where we would see it.

@Ian2012
Copy link
Contributor Author

Ian2012 commented Nov 8, 2023

This work would require a refactor in the CSS so that it will be hidden behind a modal for every other xBlock unless it's the feedback xBlock itselft.

Another thing to keep in mind is that it will only be applied to the Vertical components (units), so not much of the content will be modified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants