Closed Bug 1197865 Opened 5 years ago Closed 5 years ago

[User Story] Pin Site (via pin page dialog)

Categories

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.5+)

RESOLVED FIXED
feature-b2g 2.5+

People

(Reporter: benfrancis, Assigned: apastor)

References

Details

(Keywords: feature, Whiteboard: [systemsfe])

User Story

As a user I want to pin a site so I can re-use it often.

Specification attached.

Visual Specs:
https://mozilla.box.com/s/1ddr09r84l5i43vmi4kol3ht3gkq6ap5

Image assets (The arrows) can be found here:
https://mozilla.box.com/s/58mqrur7qckj7i3r2k0kejvs1jesxg4q

Motion can be found here on box:
https://mozilla.box.com/s/jyjjjpifhfi5pj0f2cfgqzc8vfxylum0

Attachments

(4 files, 2 obsolete files)

No description provided.
feature-b2g: --- → 2.5+
User Story: (updated)
User Story: (updated)
Summary: [User Story] Pin Site (via browser menu) → [User Story] Pin Site (via pin page dialog)
Duplicate of this bug: 1197867
Assignee: nobody → apastor
Comment on attachment 8658856 [details] [review]
[gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

In the meanwhile I finish the functionality (the Pin site button won't work yet) let's ui-review the dialog. Eric, could you please take a look?

Thanks!
Attachment #8658856 - Flags: ui-review?(epang)
Comment on attachment 8658856 [details] [review]
[gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

Hey Alberto, looking good but needs a few changes/questions.

Pin Page Dialog
1. I'm guessing the card will update when the new design is in?
2. Spacing between 'From' and bottom section to small
3. Bottom Pin Site section seems too high
4. Pin site icon looks small
5. Title of Pin site is missing
6. General spacing seems off (but this might be due to the outdated card)

Pin Site Dialog
1. Title of site is missing
2. Icon looks small
3. Vertical spacing seems off
4. Contents seem too high

Thanks!
Attachment #8658856 - Flags: ui-review?(epang) → ui-review-
Comment on attachment 8658856 [details] [review]
[gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

I'll implement the new card in a different bug. Apart from that (the pin page panel positions will change after implementing the new card), all the comments should be addressed. Thanks!
Attachment #8658856 - Flags: ui-review- → ui-review?(epang)
Attachment #8658856 - Flags: review?(gmarty)
Comment on attachment 8658856 [details] [review]
[gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

R+ seeing as we'll address the new card design and changes in a separate bug.  Thanks Alberto!
Attachment #8658856 - Flags: ui-review?(epang) → ui-review+
Comment on attachment 8658856 [details] [review]
[gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

LGTM with a couple of nits in Github.

Also, can we stop using PNG for different pixel density assets and generalise SVG? That's what we're doing in the new homescreen and I think it saves time to everybody, not to mention scalability.
Attachment #8658856 - Flags: review?(gmarty) → review+
master: https://github.com/mozilla-b2g/gaia/commit/c60d6906344b59956f57ca663db170b2d5628c80
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1205212
Ok, I tried 20 times in gaia-try with no failures. Let's try to land it again and see what happens in b2g-inbound.

master: https://github.com/mozilla-b2g/gaia/commit/94db16e0f12d2eef906aef76e061ee9041259442
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(apastor)
Resolution: --- → FIXED
This has busted Gij10.
The same failures are on the commit's gaia run as well.
(In reply to Nigel Babu [:nigelb] from comment #14)
> This has busted Gij10.

Thanks for reporting Nigel!

Backing this out for the bustage mentioned: https://github.com/mozilla-b2g/gaia/commit/aede8622d780ec71f766a3ecccbff74c04aaba4e 

Example of failure: https://treeherder.mozilla.org/logviewer.html#?job_id=2791387&repo=b2g-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(apastor)
Resolution: FIXED → ---
(In reply to Alberto Pastor [:albertopq] from comment #13)
> Ok, I tried 20 times in gaia-try with no failures. Let's try to land it
> again and see what happens in b2g-inbound.

It appears to have been intermittent in try, and perma-fail on b2g-inbound. If you expand the Gij chunks, you can see the intermittent failures. Not sure why, possibly because try is still using b2g desktop instead of the mule?
Ok, I found the issue... It should be fine after bug 1203999 lands.
Depends on: 1203999
Flags: needinfo?(apastor)
Attachment #8662244 - Attachment is obsolete: true
Attachment #8661679 - Attachment is obsolete: true
master: https://github.com/mozilla-b2g/gaia/commit/2c14ecccd4c5b8c1217f2adc65eeee5ba75579d1
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You broke gaia-theme with this change[1] by removing gaia-label. Instead you should land gaia-theme changes inside the gaia-theme repo[2] first, then uplift to gaia.

[1] https://github.com/mozilla-b2g/gaia/commit/db727e66306630f14438c8a464d887e7551c9016#diff-4d33a1f6a61508c529c2454887e0fe20L53
[2] https://github.com/gaia-components/gaia-theme
All this (/shared/elements) changes came from bower install. Didn't change anything manually in the gaia tree. Let me know if I can somehow help to fix it. Thanks!
Flags: needinfo?(kevingrandon)
If you look at the diff, gaia-theme went from 0.5.1 to 0.4.7 for some reason. I guess that's the problem?
Flags: needinfo?(kevingrandon)
You need to log in before you can comment on or make changes to this bug.