1168948 - [User Story] Preview Card

Status: RESOLVED FIXED

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

Comment 1

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/95556050

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1179712-pin-card > mozilla-b2g:master

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

Comment 4

[Alberto Pastor [:albertopq]]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

Still tests to be added, but I can think we can start a quick UI review. Ben, could you please if everything works as expected? Thanks!

Comment 5

[Ben Francis [:benfrancis]]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

This is looking awesome!

The TLD+1 detection unfortunately doesn't work for all domains (e.g. .co.uk and .me.uk domains get highlighted incorrectly). We might have to use the Mozilla library from https://publicsuffix.org/ to do this properly. We can do that as a follow up patch if you think it's too much for this patch.

A few minor visual tweaks:
* Do you need the assets for the pin icon to go on the pin button, or can you use the ones we used in the prototype?
* Make the highlighted TLD+1 in hostname #4d4d4d instead of black, and make the whole hostname font-weight normal if it isn't already.
* If possible add a 1px #bfbfbf border to the door hanger dialog

If Eric gets to it in time he might have different views :) We might need to think about what to do in landscape as it doesn't currently fit...

Please flag me for review when you're ready :)

Comment 6

[Eric Pang [:epang] UX]

Attachment: VsD_Pinning_the_Web_Preview.pdf

Hey Alberto,

I created a quick spec with some suggestions and a layout for landscape.
I mainly agree with Ben's suggestions :).

I didn't put this in the spec but instead of an outline can we have a shadow like the ones on the web component dialogues here?

http://gaia-components.github.io/gaia-components/

I will also attach the pins for the pin button shortly.

Sorry it's so messy I didn't have much time to put it together. Let me know if you have any questions!

Comment 7

[Eric Pang [:epang] UX]

Attachment: Pin Icon

Pins for pin button

Comment 8

[Eric Pang [:epang] UX]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

Thanks for making these updates Alberto!

R- based on a few small fixes (and suggestions)

Portrait:
1. Door Hanger Arrow - looks like there's an outline around it, can the same shadow be applied to the entire door hanger?
2. Can the lock and url be centered horizontal together? Currently only the site is centered.

Landscape:
1. Horizontally center align the pin button and "from, lock and url" with the remaining space to the right of the preview.
2. Also vertically center the above with the card preview (not including the icon)

Some suggestions:

1. If the user taps the site icon in the rocket bar the door hanger closes.
1. If the user taps anywhere outside the door hanger it closes.

I expected these two things to happen but didn't, what do you guys think?

Comment 9

[Alberto Pastor [:albertopq]]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

Hey Eric, could you please take a second look?
I couldn't fix the arrow. We'll probably need an image for that, so let's do it in a follow up. I'll take a look to the suggestions in the meanwhile you review this. Thanks!

Comment 10

[Ben Francis [:benfrancis]]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

Looking good. There's a lot of work packed into this so thank you! There are still some issues to address which I've commented on on GitHub, and I think we're missing integration tests.

I'm seeing a bug where the Rocketbar expands/collapses the opposite way to what it should when you scroll, but this may be unrelated to this patch, I will check on master.

Eric, one thing to notice is that when both the back & forward buttons are enabled the dialog ends up being off the screen slightly. It's still usable in this state but I wonder if you have suggestions for improving this, maybe we can get away with reducing the width of the dialog. Not sure what our minimum resolution screen is these days...

Comment 11

[Eric Pang [:epang] UX]

Attachment: VsD_Pinning_the_Web_Preview.pdf

Thanks for the note Ben!  I've updated the spec so door hanger is more narrow.  I looked at it on the Z3 device and it seems to fit with some padding on the right edge even when both arrows are present.

Let's see how this works out.  If needed we can make more adjustments.

I've updated the size to
19.6rem (Width) x 28.6rem (Height)

Alberto, can you help update before I review?  Ping me when it's ready so you don't have to wait :)

Comment 12

[Alberto Pastor [:albertopq]]

(In reply to Ben Francis [:benfrancis] from comment #10)
> Comment on attachment 8635364 [details] [review]
> [gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master
> 
> Looking good. There's a lot of work packed into this so thank you! There are
> still some issues to address which I've commented on on GitHub, and I think
> we're missing integration tests.
> 
> I'm seeing a bug where the Rocketbar expands/collapses the opposite way to
> what it should when you scroll, but this may be unrelated to this patch, I
> will check on master.
> 
> Eric, one thing to notice is that when both the back & forward buttons are
> enabled the dialog ends up being off the screen slightly. It's still usable
> in this state but I wonder if you have suggestions for improving this, maybe
> we can get away with reducing the width of the dialog. Not sure what our
> minimum resolution screen is these days...

Hey Ben, what device did you use for testing? See my attachment on how I see the dialog with both back and forward buttons (before Eric's corrections)

Comment 13

[Alberto Pastor [:albertopq]]

Attachment: pin-dialog.png

Comment 14

[Eric Pang [:epang] UX]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

R+ Thanks Alberto!

Will fix the arrow in a follow up bug

Comment 15

[Alberto Pastor [:albertopq]]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

Fixed:

- Applied same logic for the card icon than the site icon (created bug 1188285 as a follow up to unify everything)
- Using settings listener instead of getting the pref on every click
- Added UI tests
- Fixed the app title issue
- .co.uk like domains will be fixed in a follow up
- Not showing the dialog while loading

Let me know if there is something missing. Thanks!

Comment 16

[Ben Francis [:benfrancis]]

Comment on attachment 8635364 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card2 > mozilla-b2g:master

Thanks Alberto.

Comment 17

[Ben Francis [:benfrancis]]

https://github.com/mozilla-b2g/gaia/commit/3a14891fb02627fe9a39f11b8dfa6aeca9d9ee91

Comment 18

[Kevin Grandon :kgrandon]

Sorry, had to revert this for breaking basic rocketbar functionality when scrolling. Alberto - please take a look if you can.

Revert: https://github.com/mozilla-b2g/gaia/commit/ed0b3d20e5eeb04a4b3f687f0db4f47c7cc4cd71

Comment 19

[Kevin Grandon :kgrandon]

I suppose I did not have pinning the web enabled here, so maybe that's why it broke. I'm sad we don't have any tests that caught this though =(

Comment 20

[Ryan VanderMeulen [:RyanVM]]

The new test was also failing intermittently. Please fix that as well before relanding.
https://treeherder.mozilla.org/logviewer.html#?job_id=2404113&repo=b2g-inbound

Comment 21

[Francesco Lodolo [:flod]]

Can we please start adding meaningful localization comments to properties files?

pinning.pin=Pin
pinning.pin-page=Pin Page
from=from

There's no way to know that the first one is an action (verb), used as a button label, and not a noun.

Also, that "from" makes assumptions on the grammar ("from X", X will always follow "from"), and it shouldn't. It can be worked around (e.g. "from" -> "Source:"), but only if you add proper localization notes or a link to image/spec.

Comment 22

[Francesco Lodolo [:flod]]

One more question: I tried the patch but I can't figure out how to test. Can you please confirm that the "Pin" button's width has not been tailored on English and 3 letters?

Comment 23

[Alberto Pastor [:albertopq]]

Sorry about that. Stupid mistake on https://github.com/albertopq/gaia/commit/6f1656fda1ef6187a0298fe968448542f33245c3#diff-a5276edd04276b3b632b61ba37482b00R456. We were expanding where we should collapse.

Will push a new PR with the fix.

At the other hand, sorry about the locales Francesco. Is the firs time I add new locales. Could you please point me to a good example of a 'meaningful commented' localization file? Is a link to the spec enough? Thanks and sorry again!

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1168948-pinning-preview-card3 > mozilla-b2g:master

Comment 25

[Alberto Pastor [:albertopq]]

Comment on attachment 8640359 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card3 > mozilla-b2g:master

Same than the previous one, but switching expand/collapse on https://github.com/albertopq/gaia/commit/6f1656fda1ef6187a0298fe968448542f33245c3#diff-a5276edd04276b3b632b61ba37482b00R456

Comment 26

[Francesco Lodolo [:flod]]

Example

# LOCALIZATION NOTE (pinning.pin, pinning.pin-page): These strings are used as 
# button labels. "Pin" indicates the action to pin a website.
pinning.pin=Pin
pinning.pin-page=Pin Page

# LOCALIZATION NOTE (from): is followed (on a new line) by the URL source of 
# the pin. See also: https://bug1168948.bmoattachments.org/attachment.cgi?id=8639350
from=from

Links to Bugzilla attachment are good, since they'll never become obsolete.

Some more info about localization here 
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices

Can you also answer to comment 22 (button's width)?

Comment 27

[Alberto Pastor [:albertopq]]

https://github.com/mozilla-b2g/gaia/pull/31146/files#diff-81fe86f3f20787d354c9275a837572b9R613

Thanks for checking out the buttons width. They'll adapt to the word's width, so shouldn't be a problem now.

Thanks!

Comment 28

[Ben Francis [:benfrancis]]

Sorry about the regression, I forgot I mentioned this in comment 10 but didn't follow up on it.

Comment 29

[Kevin Grandon :kgrandon]

Comment on attachment 8640359 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card3 > mozilla-b2g:master

Ben's already done a review pass here, so I would like for him to review this.

Also, I would like to see the following addressed if they have not been before landing again:

1 - Intermittent test addressed before failing - comment 20
2 - Localization concerns addressed - comment 26
3 - A test to catch the regression if possible (possibly a unit test if an integration test is too difficult)

Comment 30

[Alberto Pastor [:albertopq]]

Thanks Kevin, all 3 points have been amended:

(In reply to Kevin Grandon :kgrandon from comment #29)
> Comment on attachment 8640359 [details] [review]
> [gaia] albertopq:1168948-pinning-preview-card3 > mozilla-b2g:master
> 
> Ben's already done a review pass here, so I would like for him to review
> this.
> 
> Also, I would like to see the following addressed if they have not been
> before landing again:
> 
> 1 - Intermittent test addressed before failing - comment 20

Fixed. Tried 10 times without error.

> 2 - Localization concerns addressed - comment 26

https://github.com/mozilla-b2g/gaia/pull/31146/files#diff-81fe86f3f20787d354c9275a837572b9R613

> 3 - A test to catch the regression if possible (possibly a unit test if an
> integration test is too difficult)

https://github.com/mozilla-b2g/gaia/pull/31146/files#diff-2acc1368ee89465f0918f153ea637e3fR367

Thanks!

Comment 31

[Ben Francis [:benfrancis]]

Comment on attachment 8640359 [details] [review]
[gaia] albertopq:1168948-pinning-preview-card3 > mozilla-b2g:master

Thanks Alberto

Comment 32

[Alberto Pastor [:albertopq]]

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

Comment 33

[Ben Francis [:benfrancis]]

Attachment: Preview Card - Specification