Closed Bug 835999 Opened 8 years ago Closed 8 years ago

Work - add a topsites grid to the snapped view

Categories

(Firefox for Metro Graveyard :: Firefox Start, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: ally, Assigned: ally)

References

Details

(Keywords: uiwanted, Whiteboard: feature=work)

Attachments

(1 file, 3 obsolete files)

including coloring the tiles, similiar to bookmark tiles on main awesome screen. May require refactoring the tile coloring code out of bookmarks.js

mocks attached to bug 801000
Summary: add a topsites grid to the snapped view → Work - add a topsites grid to the snapped view
Whiteboard: feature=work
Summary: Work - add a topsites grid to the snapped view → add a topsites grid to the snapped view
Whiteboard: feature=work → [completed-elm]
Whiteboard: [completed-elm]
Assignee: nobody → ally
does this look reasonable for the grid? I'd like to make sure that's solid before I start messing with the colorizing
Attachment #708421 - Flags: feedback?(mbrubeck)
Comment on attachment 708421 [details] [diff] [review]
wip 0: grid, sized for snapped, not fullscreen

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

This looks like the right approach to me.

::: browser/metro/base/content/TopSites.js
@@ +90,3 @@
>  
>    show: function show() {
> +    this._grid.arrangeItems(9, 1);

When I apply this patch, the grid still has one row instead of 9; I haven't investigated the reasons.

::: browser/metro/theme/browser.css
@@ +690,5 @@
> +
> +/* DEBUG this doesnt appear to be working */
> +#snapped-topsites .richgrid-item-content {
> +  -moz-box-orient: horizontal;
> +}

This part is actually working fine in my build.
Attachment #708421 - Flags: feedback?(mbrubeck) → feedback+
(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Comment on attachment 708421 [details] [diff] [review]
> wip 0: grid, sized for snapped, not fullscreen
> 
> Review of attachment 708421 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like the right approach to me.
> 
> ::: browser/metro/base/content/TopSites.js
> @@ +90,3 @@
> >  
> >    show: function show() {
> > +    this._grid.arrangeItems(9, 1);
> 
> When I apply this patch, the grid still has one row instead of 9; I haven't
> investigated the reasons.
> 

Did you open the start screen and then snap it immediately? There's a naughty bug (probably in richgrd) that this does not lay out vertically unless you've {already opened, had opened when you snapped} a webpage in snapped view.
Blocks: 831894
Summary: add a topsites grid to the snapped view → Work - add a topsites grid to the snapped view
Whiteboard: feature=work
Blocks: 830505
No longer blocks: 830505
Component: General → Firefox Start
Marco, this part of the snapped Firefox Start design hasn't been implemented yet and is currently attached to closed story bug 831894.  Should we reopen that bug, or file a change bug or something for this work item?

Marking this dependent on bug 828088 because some of the implementation details might be affected if we make any significant changes there to our tile widgetry.
Depends on: 828088
Flags: needinfo?(mmucci)
Keywords: uiwanted
Thanks Matt.  I'm going to reopen the story and move it 'to be planned'

Can you have a look at it and estimate the point value for full completion.

Thanks.
Flags: needinfo?(mmucci)
Blocks: 831919
Attached patch wip functional, hacky (obsolete) — Splinter Review
- icons currently bleed into box, I don't like the way this looks.
- left debug code in for those who want to play with it
-- and theres fugly stuff from me tinkering my way through the problem
- proper layout requires calling arrangeitems() when the screen is snapped (when the AweSnap screen is inited, there are no item and no dimensions, so the column/row setting code is never reached initally)
- I'm going to see about a cleaner way to do this
Attachment #708421 - Attachment is obsolete: true
Attached patch wip, still hacky (obsolete) — Splinter Review
questions are in the comments inline. I'm particularly annoyed by whatever anon element is messing up my child selector
Attachment #734143 - Attachment is obsolete: true
Attachment #734908 - Flags: feedback?(mbrubeck)
Comment on attachment 734908 [details] [diff] [review]
wip, still hacky

I poked around in the DOM inspector, ran into presumably the same confusions you did and discovered: 

#snapped-topsites-grid > richgriditem > .richgrid-item-content {
  -moz-box-orient: horizontal;
}

...for reasons I don't understand, according to the inspector, it should be:

#snapped-topsites-grid > .richgrid-grid > richgriditem > .richgrid-item-content

.. but that div.richgrid-grid breaks the selector, even though it is a parent to richgriditems and https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/Anonymous_Content#Selectors_and_Scopes says that it should be treated as such for the purpose of selectors. 

Anyhow, looks good. We will eventually need to use a class I think, rather than a id for each grid, as I assume that each grid will need the same rules from the point of view of rendering in the narrow vertical space.
Comment on attachment 734908 [details] [diff] [review]
wip, still hacky

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

Looks good!

::: browser/metro/base/content/bindings/grid.xml
@@ +339,5 @@
>            <![CDATA[
> +            let Cu = Components.utils;
> +            //Cu.reportError("inside arrange items. itemcount="+ this.itemCount);
> +            this._rowCount = this.getAttribute("rows");
> +            this._columnCount = this.getAttribute("columns");

Let's move these assignments down to where the values are first used.

::: browser/metro/base/content/browser-ui.js
@@ +533,5 @@
>          this._adjustDOMforViewState();
> +        if (aData == "snapped") {
> +          //Cu.reportError("snapped event handler, expect grid to re-arrange");
> +          //anaaktge replace with less bleeding of implementation
> +          //details

We could clean this up by making TopSitesSnappedView observe "metro_viewstate_changed" notifications and call arrangeItems on its grid.

::: browser/metro/theme/browser.css
@@ +815,5 @@
> +/*layout tile for snapped view */
> +#snapped-topsites .richgrid-item-content {
> +/* TODO figure out what #$%^ing anon element is in the middle
> + *  messing up my child selectors
> + * #snapped-topsites > richgriditem > .richgrid-item-content {*/

It looks like sfoster found a solution/workaround for this one?
Attachment #734908 - Flags: feedback?(mbrubeck) → feedback+
To clarify:
#snapped-topsites-grid > richgriditem > .richgrid-item-content

.. does work. I just don't understand why it works.
post irc discussions & feedback
Attachment #734908 - Attachment is obsolete: true
Attachment #735969 - Flags: review?(mbrubeck)
Attachment #735969 - Flags: review?(mbrubeck) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/fff11ed20a04
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.