Skip to content

Allow setting custom unit icons - #19772

Closed
Agrendalath wants to merge 1 commit into
openedx:masterfrom
open-craft:agrendalath/allow-setting-custom-unit-icons
Closed

Allow setting custom unit icons#19772
Agrendalath wants to merge 1 commit into
openedx:masterfrom
open-craft:agrendalath/allow-setting-custom-unit-icons

Conversation

@Agrendalath

@Agrendalath Agrendalath commented Feb 8, 2019

Copy link
Copy Markdown
Member

This allows setting custom icons for each unit in Studio. By default the icon will be derived from unit's type. The custom icon can be specified in studio - please see the screenshot.

JIRA tickets: OSPR-3059

Dependencies: None

Screenshots:
options

Testing instructions:

  1. Check out this branch.
  2. Log into the studio.
  3. Create new course (or enter an existing one).
  4. Create new subsection and couple of units inside it.
  5. Click Configure (fa-gear) button next to the unit, choose custom icon from the dropdown (like on attached screenshot) and click Save.
  6. Log into the LMS and enter modified course.
  7. Enter the modified unit and check if its icon matches the chosen one.

Reviewers

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @Agrendalath! I've created OSPR-3059 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Feb 8, 2019
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. The following contexts failed:

  • jenkins/python
  • jenkins/quality
  • jenkins/bokchoy

@natabene

natabene commented Feb 8, 2019

Copy link
Copy Markdown
Contributor

@Agrendalath Thank you for your contribution. Please let me know once tests pass.

@natabene

natabene commented Feb 8, 2019

Copy link
Copy Markdown
Contributor

@marcotuts Do you think we should give this an early look?

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed needs triage labels Feb 8, 2019
@marcotuts

Copy link
Copy Markdown
Contributor

From a product standpoint this isn’t something I would recommend enabling in studio. This is an example of something handled ideally as a plug-in but not in the core.

@marcotuts marcotuts closed this Feb 8, 2019
@openedx-webhooks

Copy link
Copy Markdown

@Agrendalath 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.

@Agrendalath
Agrendalath deleted the agrendalath/allow-setting-custom-unit-icons branch April 2, 2019 20:26
@Agrendalath

Copy link
Copy Markdown
Member Author

@marcotuts, we are thinking about adding something like:

def  get_xblock_icon(xblock):
    return xblock.get_icon_class()

to XBlock to maintain compatibility with this part and to allow overriding icons with plugins.
Does this approach sound reasonable?

CC: @antoviaque

@antoviaque

Copy link
Copy Markdown
Contributor

@marcotuts @nasthagiri @Agrendalath I haven't looked into the details here, but before rejecting a contribution to move it to a plugin, can we make sure there is a proper API in place to allow to support the feature?

To keep things sustainable on our end too, we need to make sure that we don't end up developing features in plugins that end up break during release upgrades - which will happen if we have to use or overwrite internal core methods to develop the plugin. If we can keep the PR open until we are clear on the API calls that can be used to maintain forward compatibility, that would help ensuring we don't end up without a good solution to achieve that.

(And to be clear, we can contribute those APIs as needed - we mainly need to agree on what they should be.)

CC @bradenmacdonald

@antoviaque

Copy link
Copy Markdown
Contributor

CC @johnmark @feanil too since this is related to our conversation at the conference ^

@marcotuts

Copy link
Copy Markdown
Contributor

@antoviaque - Happy to chat about how to advance the API discussions, this is something the team will be looking at in a dedicated way soon! (cc'ed @nasthagiri for visibility) In the interim, I do no believe it is valuable to keep this open though we can use this example potentially to talk through API requirements and needs in the future. fyi @johnmark , @feanil

@antoviaque

Copy link
Copy Markdown
Contributor

@marcotuts OK thanks for the answer!

In the meantime, how do we address @Agrendalath 's needs here? How can he get an answer on his question, to be able to complete the development he is working on, before the project ends?

@antoviaque

Copy link
Copy Markdown
Contributor

NB: See also a related situation in https://github.com/edx/edx-platform/pull/16669#issuecomment-483055730

@Agrendalath

Copy link
Copy Markdown
Member Author

@marcotuts, are there any updates on this?

1 similar comment
@Agrendalath

Copy link
Copy Markdown
Member Author

@marcotuts, are there any updates on this?

@natabene

natabene commented May 6, 2019

Copy link
Copy Markdown
Contributor

@Agrendalath I will check and let you know if I learn anything.

@Agrendalath

Copy link
Copy Markdown
Member Author

Great, thank you @natabene!

@wittjeff wittjeff left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In cms/templates/js/icon-editor.underscore:
For accessibility, the icon dropdown needs a label that is semantically connected to the control. Please add:

  • id="icon-label" to the h3 above the select.
  • aria-labelledby="icon-label" to the select.

@marcotuts

Copy link
Copy Markdown
Contributor

I don’t know that I can provide any more specific feedback on the approach toward maintaining compatibility. We are looking to build a new micro front end for the course experience, which would hopefully make this contribution easier to envision as a plugin in that application. This doesn’t help with a specific API / approach though, at least not initially.

I know @nasthagiri and others are putting time into distinctions within the world of APIs and how they would function. I don’t think we are in a place to specifically address this contribution’s needs though.

If there’s anything else you feel blocked by wrt to this PR let us know. Thanks!

@antoviaque

Copy link
Copy Markdown
Contributor

@marcotuts

I know @nasthagiri and others are putting time into distinctions within the world of APIs and how they would function. I don’t think we are in a place to specifically address this contribution’s needs though.

What's missing to be able to address it? It's a useful use case, because it's a concrete need, and we can help with any work related to it.

@openedx-webhooks openedx-webhooks added rejected and removed product review PR requires product review before merging labels Jan 22, 2021
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 rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants