Closed Bug 591535 Opened 11 years ago Closed 11 years ago

"Give me some help" link's iframe behavior is no longer necessary, on Bugzilla 4.0

Categories

(Bugzilla :: Query/Bug List, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: mkanat, Assigned: guy.pyrzak)

Details

Attachments

(1 file, 5 obsolete files)

As of now in 4.0, all of the displayed fields have their own help that you can see already in tooltips by hovering over them. So, the "Give me some help" link and the help.js system are no longer needed.
Flags: blocking4.0+
Before doing that, we should fix the query page to display the tooltip when hovering fields themselves, not only when hovering field labels.
Ah, it's true that the current system does that, but since we have the question-mark pointer that appears over the field label, I think the field label itself is enough. I don't think that showing the tooltip when hovering the field itself is actually something that users want--I know that for me, with the current help system, I find it rather annoying (particularly since it blocks out other parts of the UI).
Attached patch v1 (obsolete) — Splinter Review
Assignee: query-and-buglist → mkanat
Status: NEW → ASSIGNED
Attachment #472587 - Flags: review?(guy.pyrzak)
(In reply to comment #2)
> Ah, it's true that the current system does that, but since we have the
> question-mark pointer that appears over the field label, I think the field
> label itself is enough.

But how often do you hover a label and see the pointer appearing as a question-mark? There is no hint anywhere mentioning that to get some help, the user should hover the field labels. I'm not saying this should block this bug, but that's something we could mention.
  Sure, I get what you're saying, but I don't really want to clutter up the page with anything else, if we can avoid it. The fact that the cursor changes is enough discoverability, I think, although pyrzak would be more of the expert in that area.
I agree that a lot of the code isn't necessary. However, I don't think (based on my own experience) that mouse overs/tooltips are super discoverable. I think we should keep the "give me some help" text and replace the reload with a popup or tooltip on click, which tells people to mouse over on the labels to get information about the fields they have questions about.
Comment on attachment 472587 [details] [diff] [review]
v1

Okay, that makes sense. Perhaps we should move the link elsewhere, also?
Attachment #472587 - Flags: review?(guy.pyrzak)
Summary: "Give me some help" link is no longer necessary, on Bugzilla 4.0 → "Give me some help" link's iframe behavior is no longer necessary, on Bugzilla 4.0
we might be able to think hard and come up with one, but i think it's fine for now. It's not so bad that it should block the release IMO
(In reply to comment #8)
> we might be able to think hard and come up with one, but i think it's fine for
> now. It's not so bad that it should block the release IMO

  One what? (Was this comment on the wrong bug?)
Hrm, to clarify....

your question was: 

"perhaps we should move a link elsewhere, also?"

I responded with:

we might be able to think hard and come up with one (a new location for the link), but i think it's fine for
now. It's(the current location of the link) not so bad that it should block the release IMO
Ah, okay. :-)
Assignee: mkanat → guy.pyrzak
Attached patch v2 (obsolete) — Splinter Review
feel free to just rewrite the help text itself. I'm not too happy with it.
Attachment #472587 - Attachment is obsolete: true
Attachment #481138 - Flags: review?(mkanat)
Comment on attachment 481138 [details] [diff] [review]
v2

Well, I haven't looked this over in detail yet, but raw text can't be in .js files--it has to be in the templates, so that it can be localized.

Also, I really think we should go with something simpler. Why not just an alert or a hidden div?
Attachment #481138 - Flags: review?(mkanat) → review-
I can do a hidden div. The code was already there so I just reused it.
(In reply to comment #14)
> I can do a hidden div. The code was already there so I just reused it.

  Yeah, I understand that. I was just thinking of this in the context of, "If we were newly-implementing something like this, would we want all that code complexity for it?" and I think that we wouldn't. Also, since it's going to be such a small feature going forward, I think that it should be as simple as possible. (Also, I'm always a fan of deleting code. :-) )
Hey hey. Any chance that we can get an updated patch for this bug? Soon it's going to be one of the last 4.0 blockers.
Attached patch v3 (obsolete) — Splinter Review
Attachment #481138 - Attachment is obsolete: true
Attachment #486232 - Flags: review?(mkanat)
Attachment #486232 - Flags: review?(LpSolit)
Attached patch v4 (obsolete) — Splinter Review
sorry i was dumb, should have used css instead of JS. I'm sure if i tried i could even do this all with just CSS. Let me know if that's what you'd prefer.
Attachment #486232 - Attachment is obsolete: true
Attachment #486235 - Flags: review?(mkanat)
Attachment #486235 - Flags: review?(LpSolit)
Attachment #486232 - Flags: review?(mkanat)
Attachment #486232 - Flags: review?(LpSolit)
(In reply to comment #18)
> sorry i was dumb, should have used css instead of JS. I'm sure if i tried i
> could even do this all with just CSS. Let me know if that's what you'd prefer.

How? Using e.g. the :hover pseudo-class? If you can do it, that would be great, except that everybody would see the help being displayed.
Comment on attachment 486235 [details] [diff] [review]
v4

OK, I tested your patch, and I'm pretty sure this can be done with :hover only, or something equivalent. This means js/help.js could die entirely.

Your patch has two bugs anyway:
- Once you *click* on a label and come back, the JS code no longer works.
- When you click on the "Give me some help" link, the summary field flies to the right, moving outside the browser window.
Attachment #486235 - Flags: review?(mkanat)
Attachment #486235 - Flags: review?(LpSolit)
Attachment #486235 - Flags: review-
The second bug I know about, the first bug, I'll need to know which browser you're using, it worked fine in chrome after clicking the label a bunch of times.

As far as only doing it with :hover only, i think you're correct, and I'll try that next. I just wanted to get forward progress on this.
Attached patch V5 (obsolete) — Splinter Review
css only version. I'm pretty happy with it
Attachment #486235 - Attachment is obsolete: true
Attachment #486357 - Flags: review?(mkanat)
Attachment #486357 - Flags: review?(LpSolit)
(In reply to comment #21)
> The second bug I know about, the first bug, I'll need to know which browser
> you're using

Firefox 3.6.11 on Linux.
doesn't matter now, it's all CSS so it should work on all browsers that support the :first-child and :hover psuedo-classes.
Comment on attachment 486357 [details] [diff] [review]
V5

Great patch! :) And works fine in Firefox, Google Chrome, Opera, Konqueror, Safari, IE8 and IE9. But it doesn't work in IE6. But I don't think we should prevent this patch from being used for a 10 years old browser. r=LpSolit
Attachment #486357 - Flags: review?(mkanat)
Attachment #486357 - Flags: review?(LpSolit)
Attachment #486357 - Flags: review+
Maybe for IE6, we could display the div in place of the "Give me some help link". It's just a longer sentence, after all, but it won't break the UI.
Flags: approval4.0+
Flags: approval+
Comment on attachment 486357 [details] [diff] [review]
V5

A div cannot be a child of a span.
Attachment #486357 - Flags: review-
Oh, and IE6 almost certainly does not support first-child.
Also, do you really want this to appear on hover, instead of on click? That means that the layout will move any time somebody accidentally moves their mouse over that link, right?
(In reply to comment #29)
I thought about div being a child of span, i'm happy to make it a div with display inline.

i can remove the first child thing and put in a class.

And it will show up whenever someone mouses over the text itself and only the text "give me some help". you can see a previous version that uses JS and a click. 

Basically you guys can pick what you prefer. I've shown both options and am indifferent. attachment 486232 [details] [diff] [review] uses simplified JS instead of CSS.

I could also combine them and use simpler JS (no xy stuff). Whatever you guys want works for me.
(In reply to comment #29)
> Also, do you really want this to appear on hover, instead of on click? That
> means that the layout will move any time somebody accidentally moves their
> mouse over that link, right?

No, the layout doesn't move when hovering the link.
We should consider the CSS-only approach. We really don't need JS for that at all.
Flags: approval4.0+
Flags: approval+
mkanat, wouldn't things be easier with just a sentence of the form "Hover field labels to get help" in place of "Give me some help", and drop CSS/JS completely?
(In reply to comment #33)
> mkanat, wouldn't things be easier with just a sentence of the form "Hover field
> labels to get help" in place of "Give me some help", and drop CSS/JS
> completely?

  Sure, as long as it's unobtrusive, that sounds reasonable.
I'd go with the sentence, "Hover your mouse over the each field label to get help for that field."
Attached patch v6Splinter Review
Attachment #486357 - Attachment is obsolete: true
Attachment #489034 - Flags: review?(mkanat)
Attachment #489034 - Flags: review?(LpSolit)
Comment on attachment 489034 [details] [diff] [review]
v6

>=== modified file 'template/en/default/search/search-advanced.html.tmpl'

>+<p id="search_help">Hover your mouse over the each field label to get help for that field.</p>

Should this be "over each..." instead of "over the each..."? Otherwise looks good to me. r=LpSolit
Attachment #489034 - Flags: review?(LpSolit) → review+
Comment on attachment 489034 [details] [diff] [review]
v6

Looks good to me. :-)
Attachment #489034 - Flags: review?(mkanat) → review+
And of course, yeah, remove the extra "the" on checkin, as LpSolit pointed out.
Flags: approval4.0+
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
deleted js/help.js
deleted skins/standard/help.css
modified template/en/default/search/search-advanced.html.tmpl
Committed revision 7599.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
deleted js/help.js
deleted skins/standard/help.css
modified template/en/default/search/search-advanced.html.tmpl
Committed revision 7479.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.