Heather Arthur [:harth] Reporter
	Description • 12 years ago

To show that there's content in a node without needing to expand it, we should indicate with a "...", like Chrome.

It should be a grey or something that's not the same color as text content.

<div class="content">...</div>

	Girish Sharma [:Optimizer] Assignee
	Comment 1 • 12 years ago

using an ellipses with surrounding space, as simply using ellipses was too narrow.

IMO, "..." feels better, but not sure to use that or not.

	Dave Camp (:dcamp)
	Comment 2 • 12 years ago

Comment on attachment 703329 [details] [diff] [review]
patch v1.0

Review of attachment 703329 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/markupview/MarkupView.jsm
@@ +787,5 @@
> +    this.editor.summaryElt.addEventListener("click", function(evt) {
> +      this.markup.navigate(this);
> +
> +      if (this.expanded) {
> +        this.markup.collapseNode(this.node);

Is this ever going to happen?  The summary element is hidden when expanded, no?

@@ +837,5 @@
>        this.expander.setAttribute("expanded", "");
>        this.children.setAttribute("expanded", "");
> +      if (this.editor.summaryElt) {
> +        this.editor.summaryElt.style.display = "none";
> +      }

The other nodes here use an expanded attribute with a :not([expanded]) rule in markup-view.css.  I don't particularly care whether we use direct manipulation or attribute+style here, but would be nice to be consistent.

	Girish Sharma [:Optimizer] Assignee
	Comment 3 • 12 years ago

(In reply to Dave Camp (:dcamp) from comment #2)
> Comment on attachment 703329 [details] [diff] [review]
> patch v1.0
> 
> Review of attachment 703329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/markupview/MarkupView.jsm
> @@ +787,5 @@
> > +    this.editor.summaryElt.addEventListener("click", function(evt) {
> > +      this.markup.navigate(this);
> > +
> > +      if (this.expanded) {
> > +        this.markup.collapseNode(this.node);
> 
> Is this ever going to happen?  The summary element is hidden when expanded,
> no?
> 
> @@ +837,5 @@
> >        this.expander.setAttribute("expanded", "");
> >        this.children.setAttribute("expanded", "");
> > +      if (this.editor.summaryElt) {
> > +        this.editor.summaryElt.style.display = "none";
> > +      }
> 
> The other nodes here use an expanded attribute with a :not([expanded]) rule
> in markup-view.css.  I don't particularly care whether we use direct
> manipulation or attribute+style here, but would be nice to be consistent.

Agreed on both. reattaching.

	Girish Sharma [:Optimizer] Assignee
	Comment 4 • 12 years ago

comments addressed.

	Girish Sharma [:Optimizer] Assignee
	Comment 5 • 12 years ago

Before landing, please make a note that this patch uses " … " instead of "...".

I personally like "..." though :)

	Paul Rouget [:paul]
	Comment 6 • 12 years ago

Why is this patch a mix of 2 bugs? Please split that in 2 patches in 2 different bugs.

	Girish Sharma [:Optimizer] Assignee
	Comment 7 • 12 years ago

carry forward r+
removed the other bug's patch from this one.

	Girish Sharma [:Optimizer] Assignee
	Comment 8 • 12 years ago

my editor has some serious indentation issues -_-

fixed an unintentional indentation.

	Paul Rouget [:paul]
	Comment 9 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/2bff5a9d9783

	Tim Taubert [:ttaubert] (inactive)
	Comment 10 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/2bff5a9d9783