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

Status: RESOLVED FIXED

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

Comment 1

[Ben Francis [:benfrancis]]

Attachment: Pin Site (via pin page dialog) - specification

Comment 3

[Ben Francis [:benfrancis]]

Attachment: Preview Pinned Site (via pin page dialog) - Specification

Merged with bug 1197867

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

Comment 5

[Alberto Pastor [:albertopq]]

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!

Comment 6

[Eric Pang [:epang] UX]

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!

Comment 7

[Alberto Pastor [:albertopq]]

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!

Comment 8

[Eric Pang [:epang] UX]

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!

Comment 9

[Guillaume Marty [:gmarty]]

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.

Comment 10

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/c60d6906344b59956f57ca663db170b2d5628c80

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/49d013cdb7527315d5b94598d1e0dccd3e725262 for Gij(10) failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2774818&repo=b2g-inbound

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1197865-pin-site-browser > mozilla-b2g:master

Comment 13

[Alberto Pastor [:albertopq]]

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

Comment 14

[Nigel Babu [:nigelb]]

This has busted Gij10.

Comment 15

[Nigel Babu [:nigelb]]

The same failures are on the commit's gaia run as well.

Comment 16

[Kevin Grandon :kgrandon]

(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

Comment 17

[Kevin Grandon :kgrandon]

(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?

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1197865-pin-site-browser2 > mozilla-b2g:master

Comment 19

[Alberto Pastor [:albertopq]]

Ok, I found the issue... It should be fine after bug 1203999 lands.

Comment 20

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:albertopq-1197865-pin-site-browser4 > mozilla-b2g:master

Comment 21

[Alberto Pastor [:albertopq]]

master: https://github.com/mozilla-b2g/gaia/commit/2c14ecccd4c5b8c1217f2adc65eeee5ba75579d1

Comment 22

[Kevin Grandon :kgrandon]

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

Comment 23

[Alberto Pastor [:albertopq]]

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!

Comment 24

[Kevin Grandon :kgrandon]

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?