Closed Bug 1144849 Opened 5 years ago Closed 5 years ago

[Add-On Manager] Support renaming add-ons in the activity detail view

Categories

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

x86
macOS
defect

Tracking

(b2g-master unaffected)

RESOLVED FIXED
Tracking Status
b2g-master --- unaffected

People

(Reporter: justindarc, Assigned: yzen, NeedInfo)

References

Details

(Whiteboard: [spark])

Attachments

(4 files, 1 obsolete file)

Add-ons should be able to be renamed when viewed coming in directly from an activity (Bug 1144848).
Assignee: nobody → jdarcangelo
Priority: -- → P2
Whiteboard: [lightsaber]
This will not go on master.
Assignee: jdarcangelo → yzenevich
No longer blocks: 1144848
Priority: P2 → P1
See Also: → 1144848
Attached is a spec for the "rename" function. User tabs on text button to open up keyboard and text input box (please reference P2P sharing app -> Share My Apps -> "rename device").

Thanks
Comment on attachment 8588907 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber

Just wanted to get some feedback about the layout and styles of the description section (that includes Rename button).
Attachment #8588907 - Flags: a11y-review?(amlee)
Doug, Justin, perhaps you have a suggestion, the UI described for renaming is basically gaia-dialog (used in sharing app described in comment 2). Should I be recreating that UI without the gaia-component? Or should I be using it, granted it might need to be browserified?
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(drs)
Amy, the Settings app can't actually use the new web components yet as the rest of Gaia doesn't have the infrastructure to support it. We've been able to use WC in the Lightsaber apps because they were built outside the Gaia tree.

Could you adjust your visuals to use the old shared Building Blocks? We may eventually port the rest of Gaia over to WC, but it's not on the immediate horizon.

Yura, I would go ahead and re-implement this using BB for now. It's unfortunate, but I don't see a better way, even if this is landing on gaia/lightsaber only.
Flags: needinfo?(drs) → needinfo?(amlee)
I'm going to defer the question to Wilson, since he may know if we've already used gaia-dialog anywhere in Gaia or not. If gaia-dialog is not ready to be brought into Gaia, then I would take Doug's suggestion and just re-build it using BB.
Flags: needinfo?(jdarcangelo) → needinfo?(wilsonpage)
(In reply to Doug Sherk (:drs) (use needinfo?) from comment #6)
> Amy, the Settings app can't actually use the new web components yet as the
> rest of Gaia doesn't have the infrastructure to support it. We've been able
> to use WC in the Lightsaber apps because they were built outside the Gaia
> tree.

I don't understand what this means. Why can't the Settings app use web-components?
Flags: needinfo?(wilsonpage)
Flags: needinfo?(drs)
My understanding is that gaia-header can be included in Gaia by Browserifying it with its dependencies, including gaia-component. Doing this for every component that we import would be very burdensome. On the other hand, Justin told me that the Camera app can actually import dependencies using Bower, but that they then have to be checked into the tree.

Neither option seems ideal to me, so it seems to make more sense to just wait until Gaia has better support for pulling in dependencies. If you disagree or I'm misunderstanding something, I'd be happy to include WC, as it would make things a lot easier for everyone.
Flags: needinfo?(drs)
(In reply to Yura Zenevich [:yzen] from comment #4)
> Comment on attachment 8588907 [details] [review]
> [gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber
> 
> Just wanted to get some feedback about the layout and styles of the
> description section (that includes Rename button).

Had a 1-on-1 with Yura to review style. He's going to see if he's able to use web components.
Flags: needinfo?(amlee)
Attachment #8588907 - Flags: a11y-review?(amlee) → a11y-review+
Attachment #8588907 - Flags: review?(arthur.chen)
This is an additional workaround the delete dialog, but it uses delay to wait for the app to be installed. It is not a nice UX when the addon is actually being deleted. If only there was a way to pass/get some event attribute on uninstall/install that would indicate that it's an update.
Comment on attachment 8588907 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber

Hi Yura, I am trying to merge the settings app back to the lightsaber branch, would you mind to come up a patch against master? It is also fine if you do that after the merge.

And we are not using gaia-dialog in settings app (for keeping the style consistency with other apps). To avoid the lightsaber branch deviate too much from the master, I would suggest not to include WC for this time being.
Attachment #8588907 - Flags: review?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #12)
> Comment on attachment 8588907 [details] [review]
> [gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber
> 
> Hi Yura, I am trying to merge the settings app back to the lightsaber
> branch, would you mind to come up a patch against master? It is also fine if
> you do that after the merge.
> 
> And we are not using gaia-dialog in settings app (for keeping the style
> consistency with other apps). To avoid the lightsaber branch deviate too
> much from the master, I would suggest not to include WC for this time being.

Hi Arthur, I am using gaia-gialog as it was one of the visual requirements to be in sync and consistent with other lightsaber related apps like Sharing, etc (see Amy's comment #2). In case that's too much overhead, I'll switch to DialogService.prompt in settings.
Flags: needinfo?(arthur.chen)
Blocks: 1154408
Hi Yura, I incline to make the style consistency be kept within an app instead of a feature. Please use DialogService for this time being.
Flags: needinfo?(arthur.chen)
Whiteboard: [lightsaber] → [ignite]
Comment on attachment 8588907 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber

Hi Arthur, I updated the PR to use DialogServices instead, hopefully it looks good. Also I was talking to Doug about moving things into master from lightsaber, I'm fine moving addon-details stuff into master either after this lands on lightsaber along with other changes from the branch, but can do it as part of this bug too. Let me know. Thanks!
Attachment #8588907 - Flags: review?(arthur.chen)
Comment on attachment 8588907 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber

Thanks for the patch, Yura. In general, the patch looks good. Please check my comments in github. And I found that addon.export always fails with a DOMError on my device, is a special gecko required for this feature?
Attachment #8588907 - Flags: review?(arthur.chen)
(In reply to Arthur Chen [:arthurcc] from comment #16)
> Comment on attachment 8588907 [details] [review]
> [gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber
> 
> Thanks for the patch, Yura. In general, the patch looks good. Please check
> my comments in github. And I found that addon.export always fails with a
> DOMError on my device, is a special gecko required for this feature?

Yes, it was necessary to update to latest gecko with dev flag on. Also it depends on which addon you are exporting, pre-installed ones won't be exported I would think.
Comment on attachment 8588907 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber

All comments addressed, hopefully looks good now.
Attachment #8588907 - Flags: review?(arthur.chen)
Comment on attachment 8588907 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber

I was wrong about the lazy load thing. I just realized that jszip still got merged with the asynchronous syntax. Please add a new setting 'jszip': 'empty:' in settings.build.jslike specifying jszip should be excluded in any merge process.

r=me with the comments addressed, thanks.
Attachment #8588907 - Flags: review?(arthur.chen) → review+
Comment on attachment 8596021 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:lightsaber

Carrying over Amy's and Arthur's r+'s
Attachment #8596021 - Flags: ui-review+
Attachment #8596021 - Flags: review+
Attachment #8588907 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8596727 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:master

Same PR to master
Attachment #8596727 - Flags: review?(ejchen)
Attachment #8596727 - Flags: review?(arthur.chen)
Comment on attachment 8596727 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:master

The code looks good to me. There are only a few comments to be addressed before merging. Please check them in github, thanks!

As for the style part, I saw there are some visual refinements done in this patch and I would like those to be addressed at once in bug 1147092. Would you mind only including the change for adding the rename button?
Attachment #8596727 - Flags: review?(arthur.chen) → review+
Comment on attachment 8596727 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:master

Sorry, I meant to cancel the review.
Attachment #8596727 - Flags: review+
Comment on attachment 8596727 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:master

Addressed comments, removed all styling changes.
Attachment #8596727 - Flags: review?(ejchen) → review?(arthur.chen)
Comment on attachment 8596727 [details] [review]
[gaia] yzen:bug-1144849 > mozilla-b2g:master

r=me with the last comments addressed, thank you!
Attachment #8596727 - Flags: review?(arthur.chen) → review+
Whiteboard: [ignite] → [spark]
Keywords: checkin-needed
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
The design of the "Rename" button is basically a call for truncation.
https://transvision.mozfr.org/string/?entity=browser/installer/override.properties:Rename&repo=aurora

Take a look for example at Russian. Does something like "Переименование" fit in there?
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #32)
> The design of the "Rename" button is basically a call for truncation.
> https://transvision.mozfr.org/string/?entity=browser/installer/override.
> properties:Rename&repo=aurora
> 
> Take a look for example at Russian. Does something like "Переименование" fit
> in there?

The initial discussion was that Spark would only be in English for the initial release and so we did not look at truncation for other languages. Steph can you confirm?
Flags: needinfo?(swilkes)
You need to log in before you can comment on or make changes to this bug.