1041380 - [User Story] Show loading state in search app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Comment 1

[Peter Dolanjski [:pdol]]

Amir, is this something that you can help with over the next few days?

Comment 2

[Ran Ben Aharon [:ranbena] EverythingMe]

Peter, let's continue discussion over email.

Comment 3

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/23234

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8477822 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23234

Looks good, but the only thing that's weird is that the loading indicator is shown forever when the device is offline. Maybe we need to detect if the device is offline or fire off a reject() call at the right time?

Can you look into it and re-flag me for review? Thanks!

Comment 5

[Dale Harvey (:daleharvey)]

Comment on attachment 8477822 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23234

I assumed we didnt do remote searches while offline, but we already reject the promises properly so added a handler for that, good catch, cheers

Comment 6

[Ran Ben Aharon [:ranbena] EverythingMe]

A few questions if I may, Dale

1. Did you consider using the <progress> building block? http://buildingfirefoxos.com/building-blocks/progress-and-activity.html

2. I'd like to use this in Collections
* Could it be ready for vertical alignment?
* Does the calculated position take divider elements into account?
* Would it's position update if new grid items would be added?

Thanks, Ran

Comment 7

[Dale Harvey (:daleharvey)]

1. I didnt notice the building block, picked up the css from the video app, but probably best to switch it, thanks

2. For the search app I just lay it out with padding below the gaia-grid, it isnt part of the grid, part of this patch is to have gaia-grid update its height as it renders so when things are added to the grid it moves below them, its only removed one remote results are shown, if you want to vertically center it seems like that would be fine too

Comment 8

[Dale Harvey (:daleharvey)]

Updated to use the building block, thanks

Comment 10

[Kevin Grandon :kgrandon]

Comment on attachment 8477822 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23234

Left some comments on github. Could we try not setting the height of the grid directly? I don't think it's going to play well with the <from, to> arguments.

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 8477822 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23234

Set the grid height inside the provider, as an API I think the grid should properly define its own height, but it this will have less impact and works good

Comment 12

[Kevin Grandon :kgrandon]

Comment on attachment 8477822 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23234

(In reply to Dale Harvey (:daleharvey) from comment #11)
> Comment on attachment 8477822 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/23234
> 
> Set the grid height inside the provider, as an API I think the grid should
> properly define its own height, but it this will have less impact and works
> good

Ok, thanks. Thinking about it again, we could potentially move this same logic into the grid, I think it's better than trying to add the height that might get out of sync. Let's revisit this if we need something similar for smart collection loading.

Comment 13

[Dale Harvey (:daleharvey)]

Yeh we can follow this up if needed, as said I would prefer the grid set its own height, but this closes it out for now

https://github.com/mozilla-b2g/gaia/commit/9f79ffc8f284cda736d2ed64b45c11e9ac578e6a