override .sr class from keeping tooltips hidden in sequence nav

[caesar2164]

@sarina here's the quick fox to allow tooltips to show up...

[sarina]

Addresses https://edx-wiki.atlassian.net/browse/LMS-677

@talbs can you help out with this? This is an accessibility change that caused a regression in the visual display of the problem.

We need to decide how to balance accessibility with visibility. I don't like, for example, that the "Discussion Tag" appears as part of the problem name. I'm not sure if there's a way to specify something more stylized for visibility but also meeting the accessibility needs.

[caesar2164]

@sarina & @talbs:

I figured out where that "Discussion Tag" thing is coming from. It shows up on sequence items that have a discussion component below.

We need to figure out what we want to do for this, do we care about telling people about the discussion option?

[sarina]

I don't like exposing the discussion tag in text because it's not otherwise
exposed visually. I think the way it is now is sloppy. I don't mind
exposing the word video or problem as appropriate because we do visually
expose the type of the unit.
On Jul 22, 2013 6:34 PM, "caesar2164" notifications@github.com wrote:

I figured out where that "Discussion Tag" thing is coming from. It shows
up on sequence items that have a discussion component below.

We need to figure out what we want to do for this, do we care about
telling people about the discussion option?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/471#issuecomment-21380988
.

[caesar2164]

I meant, do we need to tell people that an item in sequence has discussion below...

so basically, do we just remove the text altogether, or try to find a graceful way to display that (or some other) text.

[sarina]

I don't think it is necessary or desirable to expose this information.
On Jul 22, 2013 6:51 PM, "caesar2164" notifications@github.com wrote:

I meant, do we need to tell people that an item in sequence has discussion
below...

so basically, do we just remove the text altogether, or try to find a
graceful way to display that (or some other) text.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/471#issuecomment-21381848
.

[frrrances]

@caesar2164 thanks for jumping on this fix. i made some adjustments to your fix because we try to avoid using the !important flag except when absolutely necessary, and in this case there were other options. Also, you had the sr class nested which was unnecessary. @talbs @sarina can you also take a look and see if this adjustment solves the issue? thx!

[sarina]

OK. I feel I'm being ignored. I don't think we should display "Discussion Tag". Can we get rid of that?

Otherwise your change does work. It just displays the problem name, then "Discussion Tag". I think there's no reason to display "Discussion Tag" (see my previous 3 comments on this PR). If I'm wrong, please tell me why.

[frrrances]

@sarina sorry, definitely not ignoring you! i thought that was hidden... let me take another look and see where that is coming from.

[talbs]

@frrrances, nice work in getting this UI back into shape and in front of users. The UI and the Sass/HTML behind look good from my perspective. If you wanted to, since you're now using visibility and opacity, you could add a transition definition onto this element - it may help with a smoother showing of the element, especially if you give it a little delay (~0.50s).

@caesar2164, I'd echo @frrrances' comments about the use of !important in your CSS rules - its something we try to avoid and only use in a very scoped and unavoidable way. Also, the .sr class and its related Sass mixin shouldn't be nested (and then overwrote using !important) - that should be used on the most atomic level of DOM elements, such as a single HTML element or piece of text.

Also, in general, please include one of us on any changes that include HTML, Sass/CSS in the future.

[talbs]

👍, @frrrances.

[caesar2164]

@frrrances thanks for getting to this quickly!

@frrrances & @talbs - sorry for the somewhat half-baked style fix. I guess i was under the impression this needed to go out ASAP and made some bad decisions, my bad!

@sarina - I'm pretty sure there is some python code somewhere that adds the "Discussion Tag" text to the item title, which is how that text shows up in the first place... (If I had to guess, I would say here: common/lib/xmodule/xmodule/discussion_module.py, or here: cms/djangoapps/contentstore/features/discussion-editor.py). If we want that text displayed for screenreader users but not visual users than maybe we wrap that in a span as well?

[caesar2164]

@sarina & @frrrances - ok, I found it. The "Discussion Tag" text gets added here: common/lib/xmodule/xmodule/seq_module.py, lines 96-98.

The text that gets added is defined here: common/lib/xmodule/xmodule/discussion_module.py, line 15.

[sarina]

If we want that text displayed for screenreader users

I can't make that call, but the point I've made a few times now is that we don't provide any visual cue to users that there is a discussion tag on the page. So we should not display it now for visual users.

However if there is a benefit to displaying it for screenreaders then we should obviously do that. I'm not too familiar with accessibility standards so I cannot make that call. But my repeated point is when this is merged in we should have the previous visual behavior, not something new.

[talbs]

Hi, all.

Thanks for the work in getting this on track. After looking into the rendering of "Discussion Tag" values, it looks like that text is rendered as part of a list of component names the current unit being viewed contains.

I've talked with @sarina and we think the ideal solution is to:

1. Revise the code that lists all of the component names to not display the name for a discussion component (since mostly all units can/will have discussions around them)
2. For accessibility and semantic scanning of the page, add in a heading to note that there is discussion on the page (this was primarily and poorly handled by the Discussion Tag rendering now)
3. Once those two are in place, get these tooltips turned back on ASAP

I've placed a bug into our Jira for 1. - https://edx-wiki.atlassian.net/secure/RapidBoard.jspa?rapidView=31&view=detail&selectedIssue=LMS-755 and will be adding the work for #2 on this branch. @shnayder will hopefully be able to help us get this out the door shortly to correct the previous regression.

[adampalay]

We can't (and shouldn't anyway) get rid of the default fields for discussion and html module's display names. And we don't want the discussion module's display names never to show up in the mouseover tooltips either. In studio, it's very easy to specify the name of the discussion. It was in XML authored courses that we often didn't include display_names for our discussion tags, so those are the ones we're really seeing this kind of ugly problem in.

The heart of the issue is that we need a better way of dealing with xmodule fields' default display_names. As long as there's a possibility our users will see them, they should be user friendly and not look like a bug.

I advocate that we change the default display_name for discussion xmodules from "Discussion Tag" to "Discussion". I think we should indicate when a unit has a discussion component. That way a student will know that a discussion is in a unit if it's there. The sloppiness @sarina was pointing out I think comes from the word "tag": it's technical feeling, as if the wrong thing rendered. "Discussion" says what it is; I think a much better default.

In my commits I've changed "Discussion Tag" to "Discussion". For the "Blank HTML Page" sloppiness, I also included an ugly hack that doesn't render the discussion's display_name if it's "Blank HTML Page." The reasoning is that if an XML author decided not to name his/her HTML component, it's probably not worthwhile to include it in the mouseover tooltip. Obviously this is a very, very short term solution until we work out a better one for dealing with xmodule fields default display_names. But, since this only really will pose a problem for xml courses, this problem will really go away as they do.

@sarina , @talbs , @shnayder , @markchang , @frrrances : thoughts?

[shnayder]

Makes sense to me. I want to hear from one of the designers though.

On Wed, Jul 24, 2013 at 2:03 PM, Adam notifications@github.com wrote:

We can't (and shouldn't anyway) get rid of the default fields for
discussion and html module's display names. And we don't want the
discussion module's display names never to show up in the mouseover
tooltips either. In studio, it's very easy to specify the name of the
discussion. It was in XML authored courses that we often didn't include
display_names for our discussion tags, so those are the ones we're really
seeing this kind of ugly problem in.

The heart of the issue is that we need a better way of dealing with
xmodule fields' default display_names. As long as there's a possibility our
users will see them, they should be user friendly and not look like a bug.

I advocate that we change the default display_name for discussion xmodules
from "Discussion Tag" to "Discussion". I think we should indicate when a
unit has a discussion component. That way a student will know that a
discussion is in a unit if it's there. The sloppiness @sarinahttps://github.com/sarinawas pointing out I think comes from the word "tag": it's technical feeling,
as if the wrong thing rendered. "Discussion" says what it is; I think a
much better default.

My last commit changes "Discussion Tag" to "Discussion". For the "Blank
HTML Page" sloppiness, I also included an ugly hack that doesn't render the
discussion's display_name if it's "Blank HTML Page." The reasoning is that
if an XML author decided not to name his/her HTML component, it's probably
not worthwhile to include it in the mouseover tooltip. Obviously this is a
very, very short term solution until we work out a better one for dealing
with xmodule fields default display_names. But, since this only really will
pose a problem for xml courses, this problem will really go away as they do.

@sarina https://github.com/sarina , @talbs https://github.com/talbs ,
@shnayder https://github.com/shnayder , @markchanghttps://github.com/markchang,
@frrrances https://github.com/frrrances : thoughts?

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/471#issuecomment-21503916
.

[sarina]

I was primarily concerned about how this comes out for a course that has
discussion on every. single. unit. And has 20ish units per subsection. I
just don't like displaying the same thing, over and over and over - I've
always felt that the way the integrated discussion panels were was just
part of the courseware itself, not a separate unit that I'd need to display
in a tooltip.

On Wed, Jul 24, 2013 at 2:39 PM, Victor Shnayder
notifications@github.comwrote:

Makes sense to me. I want to hear from one of the designers though.

On Wed, Jul 24, 2013 at 2:03 PM, Adam notifications@github.com wrote:

We can't (and shouldn't anyway) get rid of the default fields for
discussion and html module's display names. And we don't want the
discussion module's display names never to show up in the mouseover
tooltips either. In studio, it's very easy to specify the name of the
discussion. It was in XML authored courses that we often didn't include
display_names for our discussion tags, so those are the ones we're
really
seeing this kind of ugly problem in.

The heart of the issue is that we need a better way of dealing with
xmodule fields' default display_names. As long as there's a possibility
our
users will see them, they should be user friendly and not look like a
bug.

I advocate that we change the default display_name for discussion
xmodules
from "Discussion Tag" to "Discussion". I think we should indicate when a
unit has a discussion component. That way a student will know that a
discussion is in a unit if it's there. The sloppiness @sarina<
https://github.com/sarina>was pointing out I think comes from the word
"tag": it's technical feeling,
as if the wrong thing rendered. "Discussion" says what it is; I think a
much better default.

My last commit changes "Discussion Tag" to "Discussion". For the "Blank
HTML Page" sloppiness, I also included an ugly hack that doesn't render
the
discussion's display_name if it's "Blank HTML Page." The reasoning is
that
if an XML author decided not to name his/her HTML component, it's
probably
not worthwhile to include it in the mouseover tooltip. Obviously this is
a
very, very short term solution until we work out a better one for
dealing
with xmodule fields default display_names. But, since this only really
will
pose a problem for xml courses, this problem will really go away as they
do.

@sarina https://github.com/sarina , @talbs https://github.com/talbs
,
@shnayder https://github.com/shnayder , @markchang<
https://github.com/markchang>,
@frrrances https://github.com/frrrances : thoughts?

—
Reply to this email directly or view it on GitHub<
https://github.com/edx/edx-platform/pull/471#issuecomment-21503916>
.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/471#issuecomment-21506380
.

[caesar2164]

@sarina, @frrrances, @talbs, @adampalay, @shnayder:

What if a little icon be added if the discussion feature is enabled for the item, and then put the text ", Discussion" only in the .sr span?

[adampalay]

Again, this is a good UX question. In studio you have to add, explicitly, a discussion panel. This strongly suggests to a course author that discussions are not an integrated part of the courseware itself. Heroes didn't use discussion tags (or at least did sparingly; I can't find any).

Given the current state of course authoring, it therefor makes sense to indicate to the user when a particular unit has a discussion, because not all of them will have them. I like @caesar2164 's compromise of using an icon.

I am curious as to why discussion isn't integrated into the courseware.

[sarina]

Oh hm. Interesting. I guess I had made an assumption that integrated discussions were just there in Studio by default.

[caesar2164]

As icons and small images are being made Awesome, may I suggest this one?

[frrrances]

Yes, a discussion has to be explicitly added to a Unit in Studio, so there can be verticals without discussions. In chatting with @sarina and @talbs there was some agreement that sighted users would scan down the vertical and see the parts of the unit, but non-sighted users would have a harder time doing that. Either way, I think we are talking about several issues which overlap:

1. Icon showing one element of the vertical
2. Primary tooltip containing the NAMES of the element titles in the vertical
3. Secondary tooltip containing the TYPES of elements in the vertical

I think at a minimum, we should expose the icon in (1) to non-sighted users, or something equivalent, which could be (3) in a more verbose format. Ideally, we would expose (3) to non-sighted users in the span with a class for screenreaders, and possibly to all users maybe in parentheses, though this could become a very big tooltip and so I would drop this first. And it makes sense to have the tooltip expose (2) visually on hover.

I think where some of the confusion is coming in is when Authors don't name things. You can see I named the discussion in my screenshot and it looks fine. I think it does make sense to change the default language to be just "Discussion" as Discussion Tag does look like something is exposed that shouldn't be.

[frrrances]

I'm not sure if icons are the right solution... are discussions special enough to call out with an icon? Should we have icons for all types of elements in the vertical? Will the icon in the tooltip be confusing with a different icon in the tooltip?

[markchang]

@caesar2164 I would strongly advocate against an icon. They don't have a place within a tooltip.

@adampalay @sarina discussion/forums are there all the time. The in-context discussion view must be instantiated, and that is the "Discussion Tag" component. The Forums are there and functional with all courses.

We should indicate the presence of a in-context discussion to non-sighted users. If we want to put the discussion presence into the tooltip, we should use "Discussion" if it isn't specified by the author.

I am fine with there by no indication of Discussion for now.

I agree with @frrrances that we should separate type versus name. I agree with @adampalay that we need to think about how to deal with display_name ... but we aren't doing that here.

[caesar2164]

@markchang - Sorry, I meant the icon within currently existing icon. (item 1 in @frrrances's image)

@frrrances - screenreaders already have that extra span, so text can be added to that for non-sighted users...

[talbs]

Thanks for the efforts/discussion, all.

I think we'd be exacerbating the problem that this nav system has now (its icon-based and doesn't communicate well, hence the tooltips that are being discussed). In the spirit of dropping off this PR in Mergetown:

• I agree with @markchang's comment, "We should indicate the presence of a in-context discussion to non-sighted users. If we want to put the discussion presence into the tooltip, we should use "Discussion" if it isn't specified by the author."
• I'm also fine with there being no visual indication of a discussion until we can do so in a more meaningful way (either through revamping that tooltip with type/status indicators as some noted above, or revising the icon-based nav system itself).

[caesar2164]

Would this be good for a quick fix for the tooltip:

<p>${item['title']}, discuss<span class="sr">, ${item['type']}</span></p>

and then later possibly when the Font Awesome stuff happens, hash through a more ideal solution?

[markchang]

@caesar2164 I don't think that overlaying another icon onto that nav element is going to help anything, unfortunately.

[adampalay]

So is where I left it okay (pending a rebase)? Discussions stay in the mouseover, either named or defaulted to "Discussion", and "Blank HTML Page" gets cut. Is the hack to cut "Blank HTML Page" too egregious? I don't want that living in the codebase for very long. Is there a more elegant way to go about it than filtering out display_names called "Blank HTML Page"?

[adampalay]

In general, yes :). The "HTML" stuff is where it gets a little tricky. The title-less html modules are often very small, maybe containing a single link to board notes. It can be unclear if it's distinct from another component. If we give it a name like "Content" or "HTML Content", it could mislead students into thinking there was content that they couldn't find. Also "HTML" is technical-sounding again. Maybe "Details" would be better? I feel like it's one thing to find default names for something as specific as a video, and another thing for what html can convey.

Ultimately, let's figure out the fastest way to get this merged. Unless we find an elegant default display name for HTML components, I think we should filter out the unnamed ones for the meantime.

[markchang]

👍 to minimal changes to get merged.

[sarina]

@adampalay what about something like "Text Page" or something for HTML pages? I've seen courses that use HTML pages to display text such as explain a pset solution, explain the previous video's content better, or give out optional "further exploration" problems. I don't think "Text Page" is great, but I do agree with your concerns about "HTML Content". However, "Details" is really incorrect for any of the above applications; something that simply indicates, this is some text, with no video or problem, would be best. Maybe just "Text"?

If we can just pick a nicer default name then you don't have to do the hacky thing.

[markchang]

What is shown now when we don't set display_name on HTML?

[shnayder]

I like "Text". Proposal is to just do that and ship it, pending a bigger
redesign.

On Wed, Jul 24, 2013 at 11:26 PM, Sarina Canelake
notifications@github.comwrote:

@adampalay https://github.com/adampalay what about something like "Text
Page" or something for HTML pages? I've seen courses that use HTML pages to
display text such as explain a pset solution, explain the previous video's
content better, or give out optional "further exploration" problems. I
don't think "Text Page" is great, but I do agree with your concerns about
"HTML Content". However, "Details" is really incorrect for any of the above
applications; something that simply indicates, this is some text, with no
video or problem, would be best. Maybe just "Text"?

If we can just pick a nicer default name then you don't have to do the
hacky thing.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/471#issuecomment-21531198
.

[sarina]

@markchang if you delete the change made in common/lib/xmodule/xmodule/seq_module.py that omits it, it'll display "Blank HTML Page" in the tooltip until you explicitly set a title.

Victor & I agree "Text" would be much better to display as the tooltip. That way the hack in common/lib/xmodule/xmodule/seq_module.py can be removed, and we can ship this, and revisit the design more fully at a later date.

[sarina]

With the push I just made, here's what the tooltips look like for a problem with 3 units - a blank HTML page with the default title, a blank HTML page with a title I explicitly gave, and a Discussion unit:

[markchang]

👍

[sarina]

Fixed failing acceptance tests, rebased, and squashed commits. Looks ready to merge by me.

😨 @talbs or @frrrances one last 👍 before we merge?

[frrrances]

✨ 👍

[jzoldak]

@adampalay @caesar2164 @sarina Looks like the change from "Discussion Tag" to "Discussion" broke cms/djangoapps/contentstore/features/discussion-editor.feature
Can someone please fix? Thx.

[sarina]

Yeah, I'm on it - that's my bad. I only addressed 2/3 of the failing acceptance tests last night.

[adampalay]

you mean cms/djangoapps/contentstore/features/discussion-editor.py? Yeah, I
see where that would have happened. I'll fix it

On Thu, Jul 25, 2013 at 2:26 PM, Jay Zoldak notifications@github.comwrote:

@adampalay https://github.com/adampalay @caesar2164https://github.com/caesar2164
@sarina https://github.com/sarina Looks like the change from
"Discussion Tag" to "Discussion" broke
cms/djangoapps/contentstore/features/discussion-editor.feature
Can someone please fix? Thx.

—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/471#issuecomment-21574318
.

[sarina]

https://github.com/edx/edx-platform/pull/502