-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Annotation Tool Clean up and Plugins #3480
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| .mark{ | ||
| width: 10px; | ||
| height: 10px; | ||
| position: absolute; | ||
| background-size: contain; | ||
| background-repeat: no-repeat; | ||
| background-position: 50% 0%; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| /*This is written to fix some design problems with edX*/ | ||
| .annotatable-wrapper .annotatable-header .annotatable-title{ | ||
| padding-top: 20px !important; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ We try to not use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So how do I get around the fact that courses.css overwrites the styles in edx-annotator.css?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there also used
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait I'm so confused about what your'e telling me to do...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi. Perhaps I can be of some help here. I agree with others that we don't want to use !important in our CSS rules if at all possible (it creates a styling shouting match that is near impossible to maintain over time). @polesye's link to CSS specificity is right on. Though courses.css is loaded/parsed after your CSS file, there are ways to have your rule be more specific and thus respected when there are two conflicted values for the same property of the same element. The rules in the line we've commented on are different than the ones noted in your comment above (
if you still need help, find the specific style rule that's overriding yours and share it here. We can craft a selector that can get the job done from there.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My ideal outcome is that we do not introduce any
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @talbs Is there any way that we can schedule a video/audio call to discuss the matter? |
||
| } | ||
|
|
||
| .annotator-wrapper .annotator-adder button { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? If you're visually hiding the element in the UI, you'll need to consider how screen readers and assistive tech will handle it (in this case I believe they'll still announce and present the button). Toggling an attribute of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're not hiding the button. The background image gets covered up by a gradient in the background (gray colored) and setting the opacity to 0 is the only way to cover up the color and leave show the actual background image.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification. Carry on :) |
||
| opacity:0; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,14 @@ | |
| font-style: italic; | ||
| } | ||
|
|
||
| .mce-floatpanel { | ||
| z-index: 700000000!important; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also agree with @polesye's concern over the use of the large and arbitrary z-index here. While I know you need to set a value that will bring these elements to the top of the UI depth-wise, I'd recommend using a smaller number and also adhere to a scale that others can understand. We happen to have Sass placeholders elsewhere that adhere to a depth scale (see https://github.com/edx/edx-platform/blob/master/lms/static/sass/base/_mixins.scss#L93). Since you're writing CSS and not Sass, I'd recommend just borrowing the appropriate z-index value (e.g. |
||
| } | ||
|
|
||
| .annotator-wrapper .mce-container { | ||
| z-index: 3000000000!important; /*To fix full-screen problems*/ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, try to avoid
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need so big
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would second the strong recommendation to avoid using !important here as well. Please see my previous comments on how to figure out being more specific instead. |
||
| } | ||
|
|
||
| .mce-container-body { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are all of these min-width properties needed? What happens to the UI if it just takes up the width/dimensions of its parent? If possible, I'd rather rely on that context rather than have several widths that are set manually here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code sets up a "popup" editor and viewer to annotate different sections of text. This code is also extending part of a vendor file. The vendor sets min-widths that are way too thin and we wanted to extend them.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I feel like a call would be best for me to describe the situation. I don't feel like you've quite grasped the context into which the code is added. I understand that there are rules and preferences in terms of how it should all be formatted and whatnot, but there are limitations to things I can do when something like a vendor file is involved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks again for the additional info. Knowing the context, this seems fine and we can disregard my initial concern. |
||
| min-width: 400px; | ||
| } | ||
|
|
@@ -25,10 +33,6 @@ | |
| min-width: 400px; | ||
| } | ||
|
|
||
| .mce-floatpanel { | ||
| z-index: 700000000!important; | ||
| } | ||
|
|
||
| div.mce-tinymce.mce-container.mce-panel { | ||
| min-width:400px; | ||
| } | ||
|
|
@@ -42,4 +46,4 @@ div.mce-tinymce.mce-container.mce-panel { | |
| .mce-ico.mce-i-rubric{ | ||
| background-image: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAA8AAAAPCAYAAAA71pVKAAAABmJLR0QA/wD/AP+gvaeTAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAB3RJTUUH3QkBBB07nraNoQAAAhZJREFUKM+NkstrE1EUxr+5c08ykztpJtVoazHBF8FgQQzonyBKEZS6FrQKLl0EXBRT0ZULJSs3oii4TyHgu90IlTaL6qouWlv7Ck1N0BSnmZk714WbPHz07M4534+Pw3eAHdTY8A9+Nd9/bshU1DpnO4HXjh2ZY2J9/OSTxHTrnP8PvJYf+BDQ6qEDaQBB43jrTusUFy4oPjsYWYzF+VS91nxLYfdhKgONaQT3W/KMxr1XY5e+qj86f8zsKYYsZ6AvjWFzA8ORHkAnwN8So7evzL/8pzMAXL/Hq8mMv1up371T7Z+/c3n9cKeuDS6Xy6dN07zLuZ56Onk2Ed2/ANJsnE/PQMpgyffle+kYzwazB1+3waVS6X48Hr9BRPB9H57nYXplFKeSt8D1Hriug9XKF0x+Lmw+ys8m2m42DOOn4zhQSsGyLOi6jqONm9isbmFVFlDbaGKx8QaB1rvdlbNhGLAsC0IIGIYBIQSy2ROQ0oOp7wOPraHXEugRvDtnzjmi0SiICEIIEBGklAB9B6cmbG0AUnrY5m73h+m6DsYYTNMEYwxEBMY0hGNVhHkcZigBO9qHlDHS7cwYg23bAIBQKAQigud7IH0XwtxDoHwEIQ9SLKx0wa7rPiaivYyxESklXNeFBg0mjyNQTQSuATMSm6ipuYt//eVcLhdeXl5+UKlUlur1upqamVAv3j3/VCyOD3VqfwF6uLp3q+vMcgAAAABJRU5ErkJggg=='); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this image assets and how is it being loaded? Please leave a comment at the least around what this is referencing (since the source file syntax/name doesn't clearly explain that).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The image asset is right there? As the url states, we've encoded an "image" in "png" format using "base64" and are including the "data:" followed by the encoded data. I'm not sure I understand the question?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the information. This format/method of referencing a |
||
| background-repeat: no-repeat; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,270 @@ | ||
| /* | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| Diacritic Annotator Plugin v1.0 (https://github.com/lduarte1991/diacritic-annotator) | ||
| Copyright (C) 2014 Luis F Duarte | ||
| License: https://github.com/lduarte1991/diacritic-annotator/blob/master/LICENSE.rst | ||
|
|
||
| This program is free software; you can redistribute it and/or | ||
| modify it under the terms of the GNU General Public License | ||
| as published by the Free Software Foundation; either version 2 | ||
| of the License, or (at your option) any later version. | ||
|
|
||
| This program is distributed in the hope that it will be useful, | ||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| GNU General Public License for more details. | ||
|
|
||
| You should have received a copy of the GNU General Public License | ||
| along with this program; if not, write to the Free Software | ||
| Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
| var _ref; | ||
| var __bind = function(fn, me){ | ||
| return function(){ | ||
| return fn.apply(me, arguments); | ||
| }; | ||
| }; | ||
| var __hasProp = {}.hasOwnProperty; | ||
| var __extends = function(child, parent) { | ||
| for (var key in parent) { | ||
| if (__hasProp.call(parent, key)) | ||
| child[key] = parent[key]; | ||
| } | ||
| function ctor() { this.constructor = child; } | ||
| ctor.prototype = parent.prototype; | ||
| child.prototype = new ctor(); | ||
| child.__super__ = parent.prototype; | ||
| return child; | ||
| }; | ||
|
|
||
| Annotator.Plugin.Diacritics = (function(_super) { | ||
| __extends(Diacritics, _super); | ||
|
|
||
| //Options will include diacritic name, picture used, baseline | ||
| Diacritics.prototype.options = null; | ||
| Diacritics.prototype.diacriticmarks = null; | ||
|
|
||
| /** | ||
| * Declares all the functions and variables that the plugin will need. | ||
| * @constructor | ||
| */ | ||
| function Diacritics(element,options) { | ||
| this.pluginSubmit = __bind(this.pluginSubmit, this); | ||
| this.updateDiacritics = __bind(this.updateDiacritics, this); | ||
| this.updateViewer = __bind(this.updateViewer, this); | ||
| this.getDiacritics = __bind(this.getDiacritics, this); | ||
| this.getPos = __bind(this.getPos, this); | ||
| this.putMarkAtLocation = __bind(this.putMarkAtLocation, this); | ||
| this.updateEditorForDiacritics = | ||
| __bind(this.updateEditorForDiacritics, this); | ||
|
|
||
| this.options = options; | ||
| this.diacriticmarks = this.getDiacritics(); | ||
| _ref = Diacritics.__super__.constructor.apply(this, arguments); | ||
| return _ref; | ||
| } | ||
|
|
||
| //example variables to be used to receive input in the annotator view | ||
| Diacritics.prototype.field = null; | ||
| Diacritics.prototype.input = null; | ||
|
|
||
| /** | ||
| * Initalizes the Plug-in for diacritic marks. It adds in the field for the mark | ||
| * and sets up listeners from the Annotator.js file to make changes as needed | ||
| */ | ||
| Diacritics.prototype.pluginInit = function() { | ||
| //Check that annotator is working | ||
| if (!Annotator.supported()) { | ||
| return; | ||
| } | ||
| var di = this.diacriticmarks; | ||
|
|
||
| //-- Editor | ||
| var self = this; | ||
| if(di != 'undefined'){ | ||
| $.each(di,function(item){ | ||
| self.field = self.annotator.editor.addField({ | ||
| //options (textarea,input,select,checkbox) | ||
| type: 'checkbox', | ||
| label: Annotator._t(item), | ||
| submit: self.pluginSubmit, | ||
| }); | ||
| }); | ||
|
|
||
| //-- Viewer | ||
| this.annotator.viewer.addField({ | ||
| load: this.updateViewer, | ||
| }); | ||
|
|
||
| this.annotator.subscribe('annotationsLoaded', this.updateDiacritics); | ||
| this.annotator.subscribe('annotationUploaded', this.updateDiacritics); | ||
| this.annotator.subscribe('annotationDeleted', this.updateDiacritics); | ||
| this.annotator.subscribe('annotationUpdated', this.updateDiacritics); | ||
| this.annotator.subscribe('annotationEditorShown', this.updateEditorForDiacritics, this.field); | ||
|
|
||
| $(window).resize(this.updateDiacritics.bind(this)); | ||
| } | ||
|
|
||
| return this.input = $(this.field).find(':input'); | ||
| }; | ||
|
|
||
| /** | ||
| * Adds or removes tag from checked/unchecked boxes of diacritics available | ||
| * @param field {Object} - element which holds editor | ||
| * @param annotation {Object} - object that contains annotation information from database | ||
| */ | ||
| Diacritics.prototype.pluginSubmit = function(field, annotation) { | ||
| var checkedItems = $(this.field).find(':input'); | ||
| var self = this; | ||
| $.each(checkedItems, function(item){ | ||
| if(typeof annotation.tags != 'undefined'){ | ||
| var index = $.inArray(checkedItems[item].placeholder, annotation.tags); | ||
| if(index != -1){ | ||
| annotation.tags.splice(index, 1); | ||
| var annotatorWrapper = $('.annotator-wrapper').first(); | ||
| var element = annotatorWrapper.find('div.' + annotation.id); | ||
|
|
||
| if(!element.length){ | ||
| element = annotatorWrapper.find('div.undefined'); | ||
| } | ||
|
|
||
| element.remove(); | ||
| } | ||
|
|
||
| if(checkedItems[item].checked === true){ | ||
| annotation.tags.unshift(checkedItems[item].placeholder); | ||
| self.putMarkAtLocation(annotation, checkedItems[item].placeholder); | ||
| } | ||
| } else { | ||
| if(checkedItems[item].checked === true){ | ||
| annotation.tags = [checkedItems[item].placeholder]; | ||
| self.putMarkAtLocation(annotation, checkedItems[item].placeholder); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| }; | ||
|
|
||
| /** | ||
| * Draws the mark above a specific annotation | ||
| * @param annotation {Object} - location where mark should go | ||
| * @param mark {string}- type of mark that should go above annotation | ||
| */ | ||
| Diacritics.prototype.putMarkAtLocation = function (annotation, mark){ | ||
| var loc = this.getPos(annotation.highlights[0]); | ||
| var alignment = this.diacriticmarks[mark][1]; | ||
| var imgurl = this.diacriticmarks[mark][0]; | ||
|
|
||
| var top; | ||
| switch(alignment){ | ||
| case 'top': | ||
| top = (loc.y-5); | ||
| break; | ||
| case 'bottom': | ||
| top = (loc.y + loc.height-5); | ||
| break; | ||
| default: | ||
| top = loc.y; | ||
| } | ||
| $('<div></div>').addClass('mark ' + annotation.id).css({ | ||
| 'top': top, | ||
| 'left': loc.x + (0.5 * loc.width) - 5, | ||
| 'background-image': 'url(' + imgurl +')', | ||
| }).appendTo('.annotator-wrapper'); | ||
| }; | ||
|
|
||
| /** | ||
| * Gets the Diacritics from the instantiation in studio | ||
| * @returns An object with the diacritics instantiated | ||
| */ | ||
| Diacritics.prototype.getDiacritics = function(){ | ||
| var diacritics = {}; | ||
| var diacriticsList; | ||
| if(typeof this.options.diacritics != 'undefined'){ | ||
| diacriticsList = this.options.diacritics.split(","); | ||
| $.each(diacriticsList, function(key, item){ | ||
| var temp = item.split(";"); | ||
| if (temp.length > 2) { | ||
| diacritics[temp[0]] = [temp[1], temp[2]]; | ||
| } | ||
| }); | ||
| } | ||
| return diacritics; | ||
| }; | ||
|
|
||
| /** | ||
| * Gets the position of a specific element given the wrapper | ||
| * @param el {Object} - element you are trying to get the position of | ||
| */ | ||
| Diacritics.prototype.getPos = function(el) { | ||
| var element = $(el), | ||
| elementOffset = element.offset(), | ||
| annotatorOffset = $('.annotator-wrapper').offset(); | ||
|
|
||
| return { | ||
| x: elementOffset.left - annotatorOffset.left, | ||
| y: elementOffset.top - annotatorOffset.top, | ||
| width: element.width(), | ||
| height: element.height() | ||
| }; | ||
| }; | ||
|
|
||
| /** | ||
| * Redraws the marks above annotations by cycling through tags | ||
| */ | ||
| Diacritics.prototype.updateDiacritics = function(){ | ||
| $('.mark').remove(); | ||
| var annotations = this.annotator.plugins.Store.annotations; | ||
| var self = this; | ||
| $.each(annotations, function(key, annotation){ | ||
| $.each(self.diacriticmarks, function(tag){ | ||
| if($.inArray(tag, annotation.tags) != -1){ | ||
| self.putMarkAtLocation(annotation, tag); | ||
| } | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Removes unnecessary field that Annotator automatically adds to popup | ||
| * @param {Object} field - the html element that represents the popup | ||
| * @param {Object} annotation - the annotation element that holds metadata | ||
| */ | ||
| Diacritics.prototype.updateViewer = function(field, annotation){ | ||
| $(field).remove(); | ||
| }; | ||
|
|
||
| /** | ||
| * Function for adding Diacritic choices to the annotator popup | ||
| * @param {Object} field - the html element that represents the popup | ||
| * @param {Object} annotation - the annotation element that holds metadata | ||
| */ | ||
| Diacritics.prototype.updateEditorForDiacritics = | ||
| function(field, annotation){ | ||
|
|
||
| // if no tags are present, no need to go through this | ||
| if (typeof annotation.tags == 'undefined'){ | ||
| return; | ||
| } | ||
|
|
||
| var inputItems = $(this.field).find(':input'); | ||
| var dictOfItems = {}; | ||
| var self = this; | ||
|
|
||
| // add each diacritic mark to a dictionary and default to off | ||
| $.each(inputItems, function(key,item){ | ||
| item.checked = false; | ||
| dictOfItems[item.placeholder] = item; | ||
| }); | ||
|
|
||
| // match tags to diacritics and check off the ones that are true | ||
| $.each(annotation.tags, function(key,tag){ | ||
| if(self.diacriticmarks[tag]){ | ||
| dictOfItems[tag].checked = true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In 262 it's created to have the name of the tag be the key. Then in 268 it is used as an easier way to set the checkbox on if that diacritic mark is already a tag. |
||
| } | ||
| }); | ||
| }; | ||
|
|
||
| return Diacritics; | ||
|
|
||
| })(Annotator.Plugin); | ||
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.
Hi, would you be more specific about what UI problems this resolves? I'm hoping others who come across this comment can see exactly where and why this is needed (and that perhaps the root problem can be addressed down the road).
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.
It's specific only to the annotator tool we've created as an XModule. There is an editor that pops up to take annotation and the rules here solve design problems with mixing annotator.css within the standard edx css.
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.
The rules are all specific to the annotator tool (all contain the class .annotator or .annotator-wrapper and its variants) so it should not affect the UI outside of this context.
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.
Thanks, can you make the comment you added the code summarize the specific issues you're resolving due to edX's LMS base CSS (e.g. "These rules are needed to make sure the annotation UI 'X', 'Y', and 'Z' elements within the edX LMS context")? I'd like future developers/designers to know what exactly you had to overcompensate for here in the UI.