Closed Bug 1128088 Opened 5 years ago Closed 2 years ago

[P2P Sharing] Visual refinements

Categories

(Firefox OS Graveyard :: Gaia::P2P Sharing, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: amylee, Unassigned)

References

Details

(Keywords: leave-open, Whiteboard: [spark])

Attachments

(9 files, 4 obsolete files)

Bug to track progress on P2P Sharing App
Hi, 

I've attached visual design edits for the p2p sharing app and the original VSD spec for reference.

Thanks!
Attached file Wireframe_P2P_Sharing.pdf (obsolete) —
Visual Spec
Assignee: nobody → drs.bugzilla
Component: Gaia::Components → Gaia
Blocks: spark-p2p-sharing
No longer blocks: spark
Summary: [Lightsaber] P2P Sharing App → [P2P Sharing] Visual refinements
Whiteboard: [whiteboard]
Whiteboard: [whiteboard] → [lightsaber]
Attached file P2P_Sharing_IXD_VSD_v1.3.pdf (obsolete) —
Updated spec. Removed screenshots from app screens since this will not be possible for v1. Increased list item spacing from web components default to 66px high.
Attachment #8557371 - Attachment is obsolete: true
Attachment #8570093 - Attachment description: P2P_Sharing_IXD_VSD_v1.2.pdf → P2P_Sharing_IXD_VSD_v1.3.pdf
Attached file P2P_Sharing_IXD_VSD_v1.3.pdf (obsolete) —
Updated spec to reference web components for list styling
Attachment #8570093 - Attachment is obsolete: true
Status: NEW → ASSIGNED
This is getting a bit unwieldy, so let's get through one part before moving on.
Attachment #8574939 - Flags: review?(jdarcangelo)
Comment on attachment 8574939 [details] [review]
Visual refinements, part 1

Overall, looks good. I left some comments on some naming conventions that I didn't like.
Attachment #8574939 - Flags: review?(jdarcangelo) → review+
https://github.com/fxos/sharing/commit/3d2110d26b618fc26c506771d9ded771b9aa77b3

(In reply to Justin D'Arcangelo [:justindarc] from comment #7)
> Comment on attachment 8574939 [details] [review]
> Visual refinements, part 1
> 
> Overall, looks good. I left some comments on some naming conventions that I
> didn't like.

I agree completely. I'll start changing these over as I hit these spots.
Keywords: leave-open
Another part. Still much more to come.
Attachment #8575639 - Flags: review?(jdarcangelo)
Comment on attachment 8575639 [details] [review]
Visual refinements, part 2

Left some comments in the PR and discussed with you on IRC the possibility of migrating our View render() pattern over to what we used in the Customizer app (separating rendering/innerHTML from initialization/this.els/this.on). Doesn't need addressed immediately though.
Attachment #8575639 - Flags: review?(jdarcangelo) → review+
Comment on attachment 8575639 [details] [review]
Visual refinements, part 2

Amy, would you go over the Sharing app in its current state and see if there's anything else that should be changed?

Some of the dimensions that you provided didn't work well, especially any in rems. I assume my base font size was wrong, but I'm using the one inherited from gaia-components/base. I tried to get them as close as possible to the spec, but some things may be off.
Attachment #8575639 - Flags: ui-review?(amlee)
Attached image P2P_sharing_icon_x1.5.png (obsolete) —
Attached is an updated icon for P2P sharing for the homescreen. Please update the homescreen icon for the demo. 

Thanks!
Attachment #8583278 - Attachment description: P2P_sharing_icon_x1.png → P2P_sharing_icon_x1.5.png
Attached file P2P Icons.zip
Attached is default app icon (x1.5, x2.25) and P2P Sharing homescreen icon (x1.5, x2.25).
Attachment #8583278 - Attachment is obsolete: true
Updated P2P Sharing spec:

1. Updated UI for rename device
2. New header colour
3. Updated app icons
4. Spacing between elements (reference Hackerplace app)
5. Links to Hackerplace and Marketplace
Attachment #8570176 - Attachment is obsolete: true
Comment on attachment 8575639 [details] [review]
Visual refinements, part 2

Hi Doug, 

I've updated the visual spec and have noted the items that have changed, notably the UI for rename device.

A lot of the components/layouts are similar to Hackerplace. I would pull the layout/assets from there (i.e sub-headers, buttons, list-view, etc.) since I would say that Hackerplace is pretty polished.

Let me know if you have any questions and flag me for another UI review when you're ready. Thanks!
Attachment #8575639 - Flags: ui-review?(amlee) → ui-review-
Whiteboard: [lightsaber] → [ignite]
Whiteboard: [ignite] → [spark]
Component: Gaia → Gaia::P2P Sharing
Wanted to add another item to the visual refinements:

Add a hairline between the install button and app name in list items to make it clearer that you are able to access the app description screen tapping on the app name (reference settings -> USB Storage).
Attachment #8615867 - Flags: review?(jdarcangelo)
Comment on attachment 8615867 [details] [review]
Visual refinements, part 3

LGTM
Attachment #8615867 - Flags: review?(jdarcangelo) → review+
Comment on attachment 8615867 [details] [review]
Visual refinements, part 3

The previous patch introduced some issues. In fixing them, the original commit diverged significantly from what was reviewed. I've also added many more fixes into this PR.

There is still more to come, but we're getting a lot closer to being done with this.
Attachment #8615867 - Attachment description: Fix button styling to take advantage of advances from Hackerplace. → Visual refinements, part 3
Attachment #8615867 - Flags: review+ → review?(jdarcangelo)
Amy, this pre-built package is the Sharing app with visual refinements up to part 3. The app is ready for a full UI review.

Bug 1133987 isn't done yet, but I don't anticipate that there will be any issues with the UI there.
Attachment #8617768 - Flags: ui-review?(amlee)
Comment on attachment 8617768 [details]
Pre-built Sharing app with visual refinements up to part 3

Instructions:
1. Install via WebIDE.
2. Reboot your phone. This is needed because we're using datastores, which require a device reboot.
Attachment #8617768 - Flags: ui-review?(jsavory)
(In reply to Doug Sherk (:drs) (use needinfo?) from comment #22)
> Comment on attachment 8617768 [details]
> Pre-built Sharing app with visual refinements up to part 3
> 
> Instructions:
> 1. Install via WebIDE.
> 2. Reboot your phone. This is needed because we're using datastores, which
> require a device reboot.

Hi Doug, 

I was not able to get the WebIDE version of the Sharing app to detect a secondary device. Although the native spark version on homescreen was able to detect the other device. Any suggestions how I can get your patch working? 

In the mean time, these are my edits for what I was able to review:

1. Loading screen icon when you first open the app is too large. Please reference our system apps for correct icon size. Also I noticed that all the loading icon graphics for spark have jagged edges to them compared to our system apps. Can this be fixed?  

2. Share My Apps screen – Please change “Rename Device” to “Rename” 

3. “Share My Apps” header isn’t centre aligned

4. Share My Apps Toggle – Right now you have to drag the toggle to enable on/off whereas in settings you just have to tap on the list item in settings to activate the toggle. I would make it easier for the user by having the hit target cover the list item. 

5. The message “There is nobody nearby on your WiFi network who is sharing anything” Can we change the wording? Maybe “No users found sharing on your wifi network” 


Thanks!
Flags: needinfo?(drs)
Attachment #8615867 - Flags: review?(jdarcangelo) → review+
Comment on attachment 8617768 [details]
Pre-built Sharing app with visual refinements up to part 3

I made some of the changes in comment 23.

https://github.com/fxos/sharing/commit/c824cc10c8b31917e1200926d9ad62adce0e0a05
Flags: needinfo?(drs)
(In reply to Amy Lee [:amylee] from comment #23)
> 3. “Share My Apps” header isn’t centre aligned

Julien, any idea why this is happening?
Flags: needinfo?(felash)
Depends on: 1174089
The root cause is because we get and keep the references to h1 at creation, and we never update them then. I filed bug 1174089 to fix this.

I checked that with this change your code is then working fine.
Flags: needinfo?(felash)
Attachment #8617768 - Flags: ui-review?(amlee) → ui-review-
Attached image Sharing_Edits_1.png
Hi Doug, 

I was able to get the Sharing app to detect the other device after restarting the app. 

Here are my additional edits:

1. Remove “Successfully downloaded Bugzilla Lite!” dialogue since we already have a toast below it.

2. There is a typo in the toast at the bottom of the screen after downloading Bugzilla Lite. “Bugilla” should be "Bugzilla"

3. When I tried to download an app that I already have installed, I get a poorly formatted pop-up dialogue message (see attached). Please remove it and replace it with a toast notification that reads "App already installed". 

4. The pop-up dialogue for download confirmation should match the styling in web components. I've highlighted the inconsistencies (see attached).

Just flag me for another UI review when all the edits are done.

Thanks!
Attached image Sharing_Edits_2.png
Edits 2
Assignee: drs → nobody
Status: ASSIGNED → NEW
Comment on attachment 8617768 [details]
Pre-built Sharing app with visual refinements up to part 3

Clearing old review flags
Attachment #8617768 - Flags: ui-review?(jsavory) → ui-review-
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.