Closed Bug 1041380 Opened 6 years ago Closed 6 years ago
[User Story] Show loading state in search app
As a user, I want to be made aware that results are loading in the search app when non-local results are still in the process of loading so that I can tell what is happening. Acceptance Criteria: 1. While e.me or Marketplace results are loading in the search app, a loading indicator is displayed, per UX spec. 2. After scrolling down to load more results from the server, a loading indicator is displayed, per UX spec. 3. After results are loaded in AC1 or AC2, the loading indicator disappears. See page 14 of v12 of Search App spec: https://mozilla.app.box.com/s/lbw2wzw3p4jvxs24k4sg See Visual spec: https://mozilla.app.box.com/s/2iocehbfaupyc4g65lkw
No description provided.
Amir, is this something that you can help with over the next few days?
Peter, let's continue discussion over email.
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 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
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
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
Updated to use the building block, thanks
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 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 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.
Attachment #8477822 - Flags: review?(kgrandon) → review+
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.