Clean up studio edit template #88

mtyaka · 2016-08-04T09:29:23Z

Description:

This PR cleans up the Studio edit template and associated JS code to:

Use field.display_name values for field labels instead of hardcoding/duplicating the names in the template.
Directly display help text under associated input field instead of title and <span class="sr">.
Replace usage of HTML id attributes with class-es. In cases where that is not possible, ensure that the id attributes are globally unique.
Use <button> elements instead of <a> elements for buttons.
Replace input placeholders with help texts visible to everyone.

Screenshots:

Before:

After:

JIRA ticket: https://openedx.atlassian.net/browse/SOL-1990

Dependencies: None

Discussions: #77 (comment)

Sandbox URLs (installed version 9845083):

You are probably mostly interested in the Studio edit functionality. Some DnDv2 blocks can be edited at: http://studio-dndv2-sandbox5.opencraft.hosting/container/block-v1:OpenCraftX+OC1+1+type@vertical+block@e3c62b19090948b8ad89d3386bff4435

Testing Instructions:

This PR does not implement any new functionality, so there is no specific feature to test.
The important thing is to ensure that the studio edit functionality continues to look and function correctly.

Reviewers:

OpenCraft: @bradenmacdonald
TNL: @cahrens and/or @staubina
~~a11y: @edx/edx-accessibility~~ Scheduled combined review for all DnDv2.1 stories: SOL-1978
Product: @sstack22

bradenmacdonald · 2016-08-04T21:45:06Z

drag_and_drop_v2/public/css/drag_and_drop_edit.css

@@ -222,6 +242,7 @@
    display: none;
 }
 .xblock--drag-and-drop--editor .items-form .row.advanced-link {
+    cursor: pointer;


This is because the <a> element has no href="" attribute, right? Is it better to do this or use href="#" + preventDefault() ?

Yes, that's right. I removed href="#" and added this CSS rule. I believe this is considered to be a better approach than href="#", which has always felt like a hack to me.

Instead of using an a-tag, you should use a button. A-tag elements should take you someplace, whereas buttons do actions. Buttons give you the correct a11y behavior, whereas you have to manually create those behaviors with other elements using aria properties.

Tabbing through the Studio editing UI, I don't see any way to even get to these "Remove" links with the keyboard. Buttons will be part of the tab order.

@cahrens Thanks, thanks makes sense. I replaced <a> elements with <button>s.

bradenmacdonald · 2016-08-04T22:16:44Z

@mtyaka Just a few minor notes/questions - thanks for this!

mtyaka · 2016-08-05T02:49:41Z

@bradenmacdonald Thanks for the review! Your comments have been addressed.

bradenmacdonald · 2016-08-05T19:18:44Z

👍 Nice improvement, @mtyaka.

I tested this:
- Made changes to the dnd components on the Sandbox, using the updated Studio editor.
- I used ?preview-lang=eo on the sandbox to test some i18n, though the edx-platform is currently only able to translate strings that already exist in the platform and does not yet always load xblock-specific strings.
- I used chrome's element inspector to copy the rendered outerHTML for each tab of the Studio editor and search for id="..." attributes that had non-unique values.
I read through the code
Includes documentation: no readme changes required. This PR adds brief documentation where it belongs - inline where the user is making changes.

cahrens · 2016-08-08T17:56:18Z

drag_and_drop_v2/drag_and_drop_v2.py

+        # of the DnDv2 block on the same page sharing the same ID values.
+        # We avoid using ID attributes in preference to classes, but sometimes we still need IDs to
+        # connect 'for' and 'aria-describedby' attributes to the associated elements.
+        id_suffix = uuid.uuid4().hex[:16]


Another way of creating unique IDs is to incorporate the XBlock id. That's what we do for capa and ORA.

@cahrens Thanks for the hint. I replaced uuid.uuid4() with block.location.html_id(). I was not aware of the html_id() function before.

cahrens · 2016-08-08T17:57:51Z

I would suggest a doc review of the field help text; it's pretty inconsistent in formatting and style. Who is the edX doc person working on Drag and Drop?

cahrens · 2016-08-08T17:59:53Z

drag_and_drop_v2/drag_and_drop_v2.py

+        # We avoid using ID attributes in preference to classes, but sometimes we still need IDs to
+        # connect 'for' and 'aria-describedby' attributes to the associated elements.
+        id_suffix = uuid.uuid4().hex[:16]
+        js_templates = js_templates.replace('{{id_suffix}}', id_suffix)
        context = {


Why are you directly replacing id_suffix on line 224 in js_templates (I'm not sure how those are used) and also passing id_suffix in the context (which is what I would expect)?

@cahrens js_templates contains Handlebars templates. Replacing id_suffix with raw string substitutions is a bit of a hack, however I think it's still better than passing the id_suffix variable to JavaScript and making sure it is present in the handlebars context every time when rendering a handlebars template.

We still have to pass the id_suffix variable in the context, because we use it inside django templates, too.

bradenmacdonald · 2016-08-08T18:30:11Z

^ @pdesjardins is doing docs for drag and drop

cahrens · 2016-08-09T00:27:10Z

drag_and_drop_v2/public/css/drag_and_drop_edit.css

@@ -254,14 +275,15 @@
 }

 .xblock--drag-and-drop--editor .remove-zone {
+    cursor: pointer;


Same note here-- don't use a-tags.

cahrens · 2016-08-09T00:42:59Z

I imagine these font sizes (12?) are too small. Hopefully an a11y review can weigh in.

cahrens · 2016-08-09T00:45:36Z

There are color contrast errors in this "blue" section.

cahrens · 2016-08-09T00:45:57Z

drag_and_drop_v2/templates/html/drag_and_drop_edit.html

+                    <label class="h3">
+                        <span>{% trans fields.item_background_color.display_name %}</span>
+                        <input class="item-background-color"
+                               placeholder="{% blocktrans with example1='blue' example2='#0000ff' %}e.g. {{example1}} or {{example2}}{% endblocktrans %}"


Again, avoid placeholders.

cahrens · 2016-08-09T00:56:16Z

There are some good improvements here! However, there is still more work to do to make the Studio UI accessible.

I'm sure a full a11y review will find things I have missed, but for now, try accessing everything in the UI just with just the keyboard-- there are several elements that one cannot currently navigate to (specifically, there are many problematic a-tags that are acting as buttons).

staubina · 2016-08-09T12:14:54Z

I will wait to review in depth until updates are made per @cahrens comments

staubina · 2016-08-09T12:19:54Z

drag_and_drop_v2/templates/html/drag_and_drop_edit.html

-                    <label for="display-borders">{% trans "Display zone borders on the image" %}:</label>
-                    <input name="display-borders" id="display-borders" type="checkbox" />
+                    <label>
+                        <span>{% trans "Display zone borders on the image" %}:</span>


Another color take a look at @cahrens comment above

staubina · 2016-08-09T12:28:44Z

There seems to be a lack of consistency in the font sizes throughout the code. Is this something that could be updated here or in a later PR?

mtyaka · 2016-08-10T06:17:31Z

Thanks for the review @cahrens and @staubina!

I pushed an updated that addresses your comments, including making font sizes more consistent. I updated the sandbox with the latest code, please take another look.

Here are some screenshots:

staubina · 2016-08-10T11:04:21Z

👍

cahrens · 2016-08-10T20:38:08Z

This is looking a lot better! Make sure that when a button is clicked, the focus goes someplace sensible. For instance, when clicking "Add an Item", focus should move to the newly created Item.

I also find it odd that when focus is on a button (both "Add an Item" and the Delete buttons), they actually look disabled.

mtyaka · 2016-08-11T04:31:46Z

Thanks @cahrens, I addressed most of your comments:

Focus goes to the new item when clicking the "Add Zone" or "Add Item" button.
Fix :hover and :focus styles so that buttons don't look disabled.
Fixed several RTL layout issues.
Added aria-hidden="true" to icons.

Perhaps beyond the scope of this PR, but note that the Continue and Cancel buttons at the bottom of the dialog continue to be a-tags.

That button HTML code was copied from the Studio (CAPA problems currently also use <a> tags) so that we can use default Studio styles and avoid having to copy/imitate the look using our own CSS. I tried switching to button elements, but that broke the styles, so I would prefer to wait until <a> are replaced with <buttons> in the Studio, after which we can simply copy the new HTML structure.

If you'd like a review of the CSS, I can ask the FED person on my team to review.

A review of the CSS would be great, but I think it would be best if it was done outside of this PR (reviewing all CSS, not just the changes in this PR).

cahrens · 2016-08-11T11:23:47Z

I understand about the Continue/Cancel buttons. When Studio was written we incorrectly used a-tags everywhere, and that is something that will be cleaned up later this year. I agree it makes sense to wait until those fixes are done.

However, when I tab to the Continue button and press Enter, focus is remaining in the button bar-- I think this is new behavior since yesterday. After Continue, keyboard focus needs to move to the top of the new settings panel. As it is, it is very easy to move keyboard focus outside of the dialog entirely (while tabbing to try to figure out where focus is), and difficult to figure out how to get back.

Also a nit about adding a little padding around this close button:

mtyaka · 2016-08-12T02:32:10Z

@cahrens

However, when I tab to the Continue button and press Enter, focus is remaining in the button bar-- I think this is new behavior since yesterday. After Continue, keyboard focus needs to move to the top of the new settings panel. As it is, it is very easy to move keyboard focus outside of the dialog entirely (while tabbing to try to figure out where focus is), and difficult to figure out how to get back.

The behavior was not new, but I should have realized and fixed it after your previous comment, sorry I missed that. It's fixed in the latest update.

Also a nit about adding a little padding around this close button.

Last commit adds a bit of extra padding on small screens.

I updated the sandbox with latest changes.

staubina · 2016-08-12T12:32:38Z

drag_and_drop_v2/drag_and_drop_v2.py

        scope=Scope.settings,
        default=1,
    )

    item_background_color = String(
        display_name=_("Item background color"),
-        help=_("The background color of draggable items in the problem."),
+        help=_("The background color of draggable items in the problem (e.g. 'blue' or '#0000ff')."),


Nit: Should we use "example" instead of e.g.? I am not sure how well e.g. translates.

staubina · 2016-08-12T12:39:42Z

The rest looks good to me for the context of the story. 👍 Pending @cahrens comments being resolved.

cahrens · 2016-08-12T17:53:06Z

drag_and_drop_v2/public/js/drag_and_drop_edit.js

@@ -137,6 +140,7 @@ function DragAndDropEditBlock(runtime, element, params) {
                        $fbkTab.addClass('hidden');
                        $zoneTab.removeClass('hidden');
                        self.scrollToTop();
+                        $zoneTab.find('input:first').select();

                        $(this).one('click', function loadThirdTab(e) {


I believe there are preventDefault calls you can remove in this file (and perhaps others) now that the a-tags have been converted to buttons.

@cahrens Thanks for pointing that out. I removed the preventDefault calls for the a-tags that are now buttons. (Taking over for @mtyaka since he is away this week.)

cahrens · 2016-08-12T18:02:37Z

👍 It would be good to remove the unneeded event.preventDefault() calls, but I don't need to take a look at this again.

- Use field.display_name instead of hardcoding field names in the template. - Display help text under each field instead of putting it in the title attribute.

ID attributes have to be unique on the document level. To avoid accidentaly having multiple elements with the same ID, we prefer to use class attributes. There are two situations where we still need an ID attribute: - Connecting <label> elements to the associated input element using the 'for' attribute - Connecting description of an element with the 'aria-describedby' attribute. In the first case, we can often wrap the associated input inside the <label> tag, so that we don't need IDs, although in some cases that is not possible because it breaks the layout. In cases when we still need to use ID attributes, we append a random string to ensure uniqueness.

- Change a elements to buttons. - Don't use placedholers. - Make font sizes more consistent. - Fix color contrasts. - Use location.html_id() built-in function.

The button would be too close to the input field on small screens.

- i18n: Use "example" instead of "e.g." - No need to "preventDefault" for clickable elements that are now <button>s

itsjeyd · 2016-08-16T16:57:45Z

Rebased 0af3fc3 onto master.

…ndDropBlock.

itsjeyd · 2016-08-17T15:36:39Z

@staubina @cahrens Thanks again for the review. Comments addressed (aside from the one about moving focus that you said could be handled as part of the a11y review, @cahrens).

@cptvitamin @clrux I've added this story to the a11y review ticket for DnDv2.1 (SOL-1978) so you can have a look at it together with the other DnDv2.1 features. (My bandwidth for working on this PR is limited this week, and since it builds on some DnDv2.1 stories that were already part of SOL-1978, it makes sense to add this PR to the combined review.)

@sstack22 This is ready for you, please have a look.

sstack22 · 2016-08-17T18:53:55Z

Looks great, 👍 on my end. Two small nits: capitalization is a bit inconsistent (Final Feedback vs. Item description, as an example). I would also suggest removing the Note at the top of the Studio Editor - I'm concerned about course teams being prompted to delete problems. As this is outside the scope of this PR, I'm fine with this being addressed separately.

itsjeyd · 2016-08-18T09:09:34Z

@sstack22 Thanks for the review! Merging the PR now.

Re: your comments about inconsistent capitalization and removing the "Note", I'll see with @bradenmacdonald whether/how we can incorporate those into the work that's left for the DnDv2.1 epic.

bradenmacdonald reviewed Aug 4, 2016
View reviewed changes

cahrens reviewed Aug 8, 2016
View reviewed changes

cahrens reviewed Aug 9, 2016
View reviewed changes

staubina reviewed Aug 9, 2016
View reviewed changes

staubina reviewed Aug 12, 2016
View reviewed changes

cahrens reviewed Aug 12, 2016
View reviewed changes

mtyaka and others added 10 commits August 16, 2016 17:45

Refactor studio edit template.

142b49c

- Use field.display_name instead of hardcoding field names in the template. - Display help text under each field instead of putting it in the title attribute.

Address review notes.

883e332

Address review comments.

9bc8c07

- Change a elements to buttons. - Don't use placedholers. - Make font sizes more consistent. - Fix color contrasts. - Use location.html_id() built-in function.

Address review comments.

cd434ea

Set focus to first input when switching tab.

7c3c2a2

Increase padding between zone input and close button.

a4466fb

The button would be too close to the input field on small screens.

Select content of field when focusing it.

dde1c6a

Address review comments.

67845ba

- i18n: Use "example" instead of "e.g." - No need to "preventDefault" for clickable elements that are now <button>s

Update translation files.

eca2554

itsjeyd force-pushed the mtyaka/studio-help branch from 0af3fc3 to eca2554 Compare August 16, 2016 16:56

Silence pylint complaint about "location" not being a member of DragA…

f0c1f02

…ndDropBlock.

itsjeyd merged commit c958bc5 into openedx:master Aug 18, 2016

bradenmacdonald deleted the mtyaka/studio-help branch August 18, 2016 20:26

itsjeyd mentioned this pull request Aug 24, 2016

Prevent overlapping drop placements #92

Merged

7 tasks

haikuginger mentioned this pull request Sep 19, 2016

[SOL-2077] Prevent 'Change Background' button from forcing form submission #105

Merged

4 tasks

Clean up studio edit template #88

Clean up studio edit template #88

Conversation

mtyaka commented Aug 4, 2016 • edited by itsjeyd Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Aug 4, 2016

mtyaka commented Aug 5, 2016

bradenmacdonald commented Aug 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Aug 8, 2016

Choose a reason for hiding this comment

cahrens commented Aug 9, 2016

cahrens commented Aug 9, 2016

Choose a reason for hiding this comment

cahrens commented Aug 9, 2016

staubina commented Aug 9, 2016

Choose a reason for hiding this comment

staubina commented Aug 9, 2016

mtyaka commented Aug 10, 2016

staubina commented Aug 10, 2016

cahrens commented Aug 10, 2016

mtyaka commented Aug 11, 2016

cahrens commented Aug 11, 2016 • edited Loading

mtyaka commented Aug 12, 2016

staubina Aug 12, 2016 • edited Loading

Choose a reason for hiding this comment

staubina commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Aug 12, 2016

itsjeyd commented Aug 16, 2016

itsjeyd commented Aug 17, 2016

sstack22 commented Aug 17, 2016

itsjeyd commented Aug 18, 2016

mtyaka commented Aug 4, 2016 •

edited by itsjeyd

Loading

cahrens Aug 9, 2016 •

edited

Loading

cahrens commented Aug 11, 2016 •

edited

Loading

staubina Aug 12, 2016 •

edited

Loading