Annotation Tool Clean up and Plugins #3480
Conversation
|
@singingwolfboy I have a quick question about how to structure these PRs. So as you can see these are from the #2515 PR divided into chunks. Each one is dependent on the previous one being completed. How do I make the branches so that I can continue from the previous chunk, but not have them also be added to this PR. For example In the commits above "Reconnecting Token Generator for Annotation Tool" is actually from PR #3466. I still want to do my coding on the basis that the first chunk has been accepted but I don't want them to be part of this PR. Any way to do that or is that the way it's going to have to be? |
|
@lduarte1991 Unfortunately, there isn't a great way to do that. The best way that I know of is to make the second PR contain all the same commits as the first PR (and then the second PR also has its own changes). The reason I suggest this is because when the first PR is merged, Github will automatically remove those first few commits from the second PR, since they are no longer in the diff between master and your branch. However, Github will only do that if the commits that get merged are the same commits as the ones in the pull request -- identical hashes. This means that if the first pull request is rebased, you'll need to rebase the second pull request to match. In the second pull request, you can also link to Github comparison view to show the comparison between two arbitrary commits: for example, between the last commit of the first pull request and the last commit of the second pull request. That will make it easier for reviewers to see the intent of the second pull request, without getting distracted by the contents of the first pull request that is contained within the second pull request. Does that make sense? The Github comparison view URL is https://github.com/edx/edx-platform/compare. |
|
@lduarte1991 Since the first PR has been merged can you rebase this branch against master and remove the commits from this branch which were in the first commit? Recently we have figured out that the tinymce.js being included globally for the annotator tool is causing some conflicts and the email editor to not work in Firefox. So we will like to get these changes merged in soon. Thanks! |
|
Just rebased and I'd love make sure this stuff gets pushed in as quickly as possible. |
|
DOC-445 |
| @@ -1,4 +1,8 @@ | |||
| /*This is written to fix some design problems with edX*/ | |||
| .annotatable-wrapper .annotatable-header .annotatable-title{ | |||
| padding-top: 20px !important; | |||
There was a problem hiding this comment.
Do we really need this !important? Can we re-write it without it?
There was a problem hiding this comment.
++ We try to not use !important anywhere in our css.
There was a problem hiding this comment.
So how do I get around the fact that courses.css overwrites the styles in edx-annotator.css?
There was a problem hiding this comment.
Is there also used !important ?
There was a problem hiding this comment.
There was a problem hiding this comment.
Wait I'm so confused about what your'e telling me to do...
As an example:
- My CSS rule is for
.annotator-editor a, .annotator-filter .annotator-filter-property label. If I take the !important out of this rule and all rules on this CSS file then the rules from courses.css overwrite it. - The classes in courses.css all have rules like
a:link, a:visitedwhich are LESS specific than mine. - Because they are more broad they overwrite my more specific rules
- According to that article it tells me only to use !important when I'm being specific in my attributes and once again, my rules are more specific than those on courses.css.
- I didn't write courses.css and those seem to involve higher and broader standards should I not provide my specific changes. Am I just being dumb and putting my css in the wrong place where it will only get loaded before and not after?
There was a problem hiding this comment.
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 (.annotatable-wrapper .annotatable-header .annotatable-title vs. .annotator-editor a, .annotator-filter .annotator-filter-property label). Assuming we're dealing with the selectors from your actual code, without knowing exactly what is overriding your padding on .annotatable-wrapper .annotatable-header .annotatable-title, my steps to resolve this would be:
- Find out what base LMS style rule (from courses.css) is trumping yours. This involves peeking at the web inspector and finding the exact selector rule and the property involved.
- Re-craft your CSS selector to be more specific than the rule you found in step 1. This could involve adding additional pseudo states such as
:visisited, :link, :activeif you're working with links or could be as easy as referring to the parent element's class attribute or a specific HTML element. - Remove the !important and comment in detail on why the style rule is needed. These will help with maintenance and others who want to build on what you have or pay down some CSS debt and remove the hurdle you jumped over.
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.
There was a problem hiding this comment.
My ideal outcome is that we do not introduce any !important statements into the CSS. You should be able to be more specific with selectors in all cases to overcome the LMS CSS.
There was a problem hiding this comment.
@talbs Is there any way that we can schedule a video/audio call to discuss the matter?
| } | ||
|
|
||
| .annotator-wrapper .mce-container { | ||
| z-index: 3000000000!important; /*To fix full-screen problems*/ |
There was a problem hiding this comment.
Again, try to avoid !important in these two rules, please.
There was a problem hiding this comment.
Do we really need so big z-index?
There was a problem hiding this comment.
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.
|
Looking pretty good! Again I think failing tests on this build have nothing to do with your work. Once you address remaining comments you'll get a new build and we can start focusing on getting a clean one. |
|
I'm still very much unclear on the |
|
@lduarte1991 I've asked our design team to advise on the CSS issue. Hopefully they will get back to us by tomorrow. |
|
@sarina Thanks a million!! |
| @@ -1,4 +1,8 @@ | |||
| /*This is written to fix some design problems with edX*/ | |||
There was a problem hiding this comment.
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.
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.
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.
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.
|
@lduarte1991, I just spoke with @sarina and she filled me on the urgency of this work. Unfortunately, my schedule is a bit booked today and early next week. While my concerns about the use of I'm fine with these things merging in for now, but would really like a clean up task/pull request for addressing these things shortly. To support that, I'm also happy to chat a bit about and help with specifics with you in the near future. When you get a clean up pull request started, let me know and I'm happy to schedule the time. A Design/CSS 👍 to 🚚 this to Mergetown with that clean-up task in the near future. |
|
@lduarte1991 I'm happy to merge this once:
Ping me when that's done, please. |
|
@lduarte1991 any movement on this? I'm trying to get these in fast per your request. |
|
Will do these soon! Sorry for the delay. — On Mon, Jun 2, 2014 at 9:56 AM, Sarina Canelake notifications@github.com
|
|
👍 |
- UI Fixes for Diacritic Mark Plugin
Making sure extension comes after annotator css
|
Great, thanks! I'll merge this once the build passes. |
|
Please make sure you get in touch with @talbs regarding the UX fixes. Feel free to email me (sarina [at] edx [dot] org) if you want some help setting that up. |
Annotation Tool Clean up and Plugins
Background: As per Christina's request I'll be turning the Pull Request #2515 into a few smaller PRs. This is PR Chunk #2. #3466 is the first chunk.
Goal of this PR: First goal is to clean up a few CSS changes: 1) The "display name" gets a little wonky with the text annotation module, there's a fix for it here only when it's within the annotator-wrapper. 2) Fixing the size of the panel for the widget. 3) Fixed the size of the dropdown for the tags. The second goal is to add the Diacritic Plugin that I've written for the annotator tool. The last is to fix the plugin for colored tags. It will allow for user-generated tags instead of only instructor-designated ones.
CMS Updates: UI Fix, the title should not be off the module anymore.
LMS Updates: User should now be able to create tags even if they are not in the dropdown menu. If the instructor wants them to use diacritic marks they will see a list of appropriate marks underneath the tags that you can check on or off. A mark should appear or disappear accordingly.
Testing notes: In studio, under the settings tab of the text annotation module there will now be an option to include diacritic marks. Use the following in order to get the "breve" symbol to show up:
breve;http://i.imgur.com/PbSLGLP.gif;topThen head over to the LMS and once you highlight you should see the "breve" option appear as one you can check. Hitting save will make that appear. "breve" will also be a "tag" but it will not disappear if you delete that tag in the editor until you check off the box.To test the tags just add whatever tag you want. If it's one from the dropdown menu it'll change to the appropriate color, otherwise it will tell you to hit ENTER to add a personal tag and it will be the default yellow highlight.