964337 - Implementation a mechanism to edit Homescreen Bookmarks

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Comment 1

[Cristian Rodriguez (:crdlc)]

Attachment: Patch v1

Comment 2

[Cristian Rodriguez (:crdlc)]

Attachment: Editable bookmarks

Hi, I've just implemented a basic style for editable bookmarks following the wireframe. I would like to know how to show it correctly, thanks a lot

Comment 3

[Przemek Abratowski [:przemek] UX]

Comment on attachment 8366044 [details]
Editable bookmarks

I'm not working on this feature. I'll forward this to Peter La.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

My assumption has always been that with bookmarks at the system level we will be able to edit them in the settings app. That will offer a much more appropriate place to do that.

Comment 5

[Cristian Rodriguez (:crdlc)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #4)
> My assumption has always been that with bookmarks at the system level we
> will be able to edit them in the settings app. That will offer a much more
> appropriate place to do that.

Vivien,

   This feature is already implemented in Homescreen, ready to review and land. Will bookmarks be ready for 1.4 in system (creating, editing, new code in homescreen to support the new implementation, test, QA, (IMHO a lot of work) etc...)? I think that we could go ahead with it and later we could do a migration of bookmarks to system but if finally we don't have time enough to afford this for 1.4 we will have this implementation in time thought

   What do you think about my approach Peter, Vivien?

Thanks

Comment 6

[Przemek Abratowski [:przemek] UX]

Forwarding to Eric.

Comment 7

[Peter Dolanjski [:pdol]]

(In reply to Cristian Rodriguez (:crdlc) from comment #5)
> 
>    What do you think about my approach Peter, Vivien?
> 

I think we could definitely ship this even if the bookmark migration doesn't happen in 1.4.  It is useful even as a standalone feature.

Comment 8

[Jason Smith [:jsmith]]

This is a targeted feature, not a committed feature. So this isn't going to block.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8366040 [details]
Patch v1

I'm a bit worried that if we land this feature we will have to make it works across releases. And editing bookmarks from the homescreen is not necessarily something that is wanted as it will create a distinction between the apps and bookmarks while we are trying to blurred it a bit.

I suggest that Christian spend some time to help Kyle to set up the bookmarks datastore on the homescreen and explorate the various things it will offers to us.

Assigning the review to Kyle for this purpose and to starts the conversation on that. (and also I'm on PTO all next week and I won't have access to a laptop).

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

We've got the bookmark datastore coming in on bug 940647, so I'll block this on that. Once that lands, we'll at least have the datastore in the system app, though we'll still need a mechanism to sync to the homescreen.

Comment 11

[Cristian Rodriguez (:crdlc)]

Honestly I don't know why we are waiting lot of time (1-27) to review and land it. I suspect that there is not any intention to do this. Sadly the time goes by and v1.4 won't have editable bookmarks. I thought that we will land it for 1.4 regarding to comment 7. 3 weeks later the review is re-assigned to another person.

Comment 12

[Cristian Rodriguez (:crdlc)]

Why this bug depends on bug 859488?

Comment 13

[Marcelino Veiga Tuimil [:sonmarce]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> I'm a bit worried that if we land this feature we will have to make it works
> across releases. And editing bookmarks from the homescreen is not
> necessarily something that is wanted as it will create a distinction between
> the apps and bookmarks while we are trying to blurred it a bit.
> 
> I suggest that Christian spend some time to help Kyle to set up the
> bookmarks datastore on the homescreen and explorate the various things it
> will offers to us.
> 
> Assigning the review to Kyle for this purpose and to starts the conversation
> on that. (and also I'm on PTO all next week and I won't have access to a
> laptop).

I do not understand why we should change this feature, as per comment 7 it is something that product team wants, so we should review current patch as is to be land it as soon as possible

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Wow. Really weird mispaste, meant for this to block on bookmark migration, which was in the URL field instead. Fixed that, sorry.

Comment 15

[Cristian Rodriguez (:crdlc)]

Rebased pull request, ready to review

Comment 16

[Eric Pang [:epang] UX]

Attachment: edit-bookmark.jpg

Hi Cristian,

I've attached a new spec for the visuals of this.  Can you please update and flag me for UI review when ready?  I'm going to remove Przemek's flag.

Let me know if you have any questions, thanks!

Comment 17

[Francis Djabri [:djabber]]

Comment on attachment 8366044 [details]
Editable bookmarks

Please update as per the visuals that Eric has added to the bug.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8366040 [details]
Patch v1

UI Change from UX team means code change, so r-'ing. Re r? me when changes are done.

Comment 19

[Cristian Rodriguez (:crdlc)]

ok, I will see it during today and/or tomorrow, thanks a lot

Comment 20

[Cristian Rodriguez (:crdlc)]

Attachment: editable label.png

Comment 21

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8366040 [details]
Patch v1

thanks

Comment 22

[Eric Pang [:epang] UX]

Comment on attachment 8378345 [details]
editable label.png

Looks good, thanks Cristian!

Comment 23

[Cristian Rodriguez (:crdlc)]

Thanks to you for the CSS rules ;)

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Just a heads up. I'm trying to figure out what's where with the version/feature shifts after last week, which is why I've stalled on reviews on this. I need to figure out what this is going to look like next to bookmarks, which is now slated for 1.5. Should hopefully have an answer ASAP though.

Comment 25

[Marcelino Veiga Tuimil [:sonmarce]]

It is true that there is some uncertainty regarding the schedule, but I think we should continue going forward the best way we can, so following the normal process of coding and reviewing, even if we cannot land in master at this moment, otherwise we would be stuck with almost no progress

Comment 26

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8366040 [details]
Patch v1

Ok. Since bookmark migration got pushed out to 1.5, I'm not going to be an expert on this code for a while. I talked to UX too, and they're happy with where this is and don't think it'll conflict with bookmarks in the future, even after we migrate them. I'm handing back to the other module owner for review.

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #13)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> > I'm a bit worried that if we land this feature we will have to make it works
> > across releases. And editing bookmarks from the homescreen is not
> > necessarily something that is wanted as it will create a distinction between
> > the apps and bookmarks while we are trying to blurred it a bit.
> > 
> > I suggest that Christian spend some time to help Kyle to set up the
> > bookmarks datastore on the homescreen and explorate the various things it
> > will offers to us.
> > 
> > Assigning the review to Kyle for this purpose and to starts the conversation
> > on that. (and also I'm on PTO all next week and I won't have access to a
> > laptop).
> 
> I do not understand why we should change this feature, as per comment 7 it
> is something that product team wants, so we should review current patch as
> is to be land it as soon as possible

Hey Marcelino,
Adding this code at this stage means we will have to support it for a longer time in the homescreen and the time spend on supporting/fixing the issue for that is some time not spent on implementing the bigger picture. There is only a few people working on the homescreen... and there is only a few people doing reviews too, which may leads to some delay when there is some more urgent subjects (like APZC, which explains my delay Christian, sorry about it).

And I have not seen any discussion with developers, or any emails threads, about where editing bookmarks should live. (Discussion that touch people working on the system, the homescreen and settings). So I'm curious about how the decision has been taken, and if it is future proof.

> it
> is something that product team wants, so we should review current patch as
> is to be land it as soon as possible

My understanding of functional teams seems to be very different than yours. 
Products wants thinks, Developers wants things, UX wants things, and all together people decide what can be done and what should be done in which order. And this is because all together are in the same boat, working on the same thing. In this process, there are priorities, which are often dictate by the markets, and where product/developers/UX are normally on the same line for a successful product.

That said, Kyle mentioned that he got the green light from UX in his last comment though. Since product, UX and a few other developers seems fine with it, it sounds good to me. (Even if I still think having put the efforts on bookmarks migration to achieve more consistency between apps and bookmarks should have been prioritized instead of this feature).

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8366040 [details]
Patch v1

Globally looks good but a few comments on github.

Comment 29

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8366040 [details]
Patch v1

Comments addressed, thanks for the review

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8366040 [details]
Patch v1

r+ with one nit.

Comment 31

[Cristian Rodriguez (:crdlc)]

Two bugs were detected by Rafa from QA in my local branch (taken for this commit) and they were fixed before landing in master.

1) "Done" button should be disabled when the form is displayed before changing nothing
2) Sometimes the UI was hidden automatically without clicking on "cancel" or "done" buttons

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/fc69af7c4c23ed5f6fc4e5c208859e10896c0b6f