Closed Bug 899434 (sdk/ui/menuitem) Opened 11 years ago Closed 7 years ago

[Tracking] Menuitem Component

Categories

(Add-on SDK Graveyard :: General, defect, P2)

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: evold, Assigned: evold)

References

(
URL
)

Details

Attachments

(2 files)

Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1215
358 bytes, text/html
irakli
: review+
Details
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1269
358 bytes, text/html
Details 
Both Sidebars and Toolbars require a menuitem, and for some reason we don't have a module for this yet.. the one I have in pathfinder is quite popular, and if I could use it in the SDK I'd have much less code and tests to write..

Fore example FirefoxOS Simulator uses this module, Collusion does too, and I'm forgetting a bunch that will come to me later.
Assignee: nobody → evold
Priority: -- → P2
Alias: sdk/ui/menuitem
Hardware: x86 → All
Summary: Need a menuitem module → Menuitem Component
Summary: Menuitem Component → [Tracking] Menuitem Component
Irakli, can you take a look at the JEP and give me your feedback before I start implementing this?
Flags: needinfo?(rFobic)
Hi Erik,

Thanks for writing this up here is some of my immediate thoughts:

> Array Integer or String attachTo: describes the parent for the menuitem. If a
> array is used then the first valid parent in the array will be used.

In places API we have notion of `group` that indicates parent, I think it may make
sense to call this field `group` as well.

I think I would suggest to rephrase as follows:

Array of group constants that menu may be placed in. Note that menu item will appear only under one, most appropriate parent from the given options. Add-on's provides is given from left to right. This enables add-ons to handle different platforms in order of their preference (note not all groups will be available
on all platforms).

I don't like that this property is string or number or array of them. I think it
should always be an array and instead of string or numbers we should expose constants with readable names representing different parent groups. This would
also align with identical approach in places API. This is also going to allow us
to start with just a few well thought out groups and later we may add more.

Another thing that I'd like to get more input on is regarding following:

# label

Is there really a use cases for changing a `label`, do you have an example ? I think I would prefer if this remained immutable as I would love if we could support
Fennec where labels can't change at runtime, so unless we have a compelling use case
I'd omit setter for this.

# visible

Not sure this is really that useful either and again it's not something supported on the Fennec. I know we could add / remove menu items to emulate visibility but
then I'd rather let users do it.

# enabled

This is another case where there's no way to support this on Fennec at least not right now. Do we really need it ?


My fennec comments are based off:
https://developer.mozilla.org/en-US/docs/Extensions/Mobile/NativeWindow


One thing that I would really want to change is the way we handle `type`. Having it as an attribute does not really scales, meaning that more types may have more
specific properties and have them around in the contexts where they don't make
sense is kind of ugly. It's in fact already a case `checked` field does not makes sense on non checkbox type menu item. Can we just have `MenuItem` and `ToogleMenuItem` (or something with a better name). This also allows us to provide
`MenuItem` on both mobile and desktop while providing `ToogleMenuItem` only on desktop (since we can't do it on mobile).

I would also add an icon field to menu item, although would only implement support for icon on mobile since it does not really makes sense on desktop and will cause
bad UX.

It also maybe interesting to add `hotkey` property where `Hotkey` instance  can be placed. If it is placed we could display the hotkey combination similar to ones in built-in keys. This very much can be a future improvement & definitely would be better to do in a second cut if ever.


Overall I think it's great, only thing that really bothers me is type & attachTo, little skeptical about `label`, the rest points where more of a doubts than issues.

BTW if we end up doing high level API I think I'd prefer to have menu items described in package.json.
Flags: needinfo?(rFobic)
Depends on: 906323
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> 
> # label
> 
> Is there really a use cases for changing a `label`, do you have an example ?
> I think I would prefer if this remained immutable as I would love if we
> could support
> Fennec where labels can't change at runtime, so unless we have a compelling
> use case
> I'd omit setter for this.

Hmm I thought this was possible on Fennec, but it appears you are right.  I'll change the JEP for now and try to change Fennec in bug 906323.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
>
> # visible
> 
> Not sure this is really that useful either and again it's not something
> supported on the Fennec. I know we could add / remove menu items to emulate
> visibility but
> then I'd rather let users do it.


This is supported by Fennec https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/menu/update so it's simple to implement.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> 
> # enabled
> 
> This is another case where there's no way to support this on Fennec at least
> not right now. Do we really need it ?


Again this is actually supported by Fennec https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/menu/update

Not sure why you said it was not.  It is used by the 'Settings' menuitem, which is disabled until it is possible to view Settings.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> I would also add an icon field to menu item, although would only implement
> support for icon on mobile since it does not really makes sense on desktop
> and will cause
> bad UX.

I'd like to leave icons out of the first round, I don't think it's very useful, and we can make that a follow up bug which would be a good first bug for someone else.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> 
> It also maybe interesting to add `hotkey` property where `Hotkey` instance 
> can be placed. If it is placed we could display the hotkey combination
> similar to ones in built-in keys. This very much can be a future improvement
> & definitely would be better to do in a second cut if ever.

How would you see this implemented Irakli? in bug 664361 you were against a xul based hotkey implementation which would be necessary here, I don't want to see two implementations of hotkeys in the sdk when it is unnecessary.  I think this functionality should be decoupled, and the hotkey module would make a hotkey that can be applied to UI components.  This way the UI component APIs are more easy to use, and implement.
Flags: needinfo?(rFobic)
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)

> BTW if we end up doing high level API I think I'd prefer to have menu items
> described in package.json.

Why?  that seems very anti-jetpack, as no other browser window UI component is required to be described in the package.json.  Also it seems painful and non-useful.  If you wanted to add a menuitem permission type, that seems more reasonable.
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #3)
> 
> One thing that I would really want to change is the way we handle `type`.
> Having it as an attribute does not really scales, meaning that more types
> may have more
> specific properties and have them around in the contexts where they don't
> make
> sense is kind of ugly. It's in fact already a case `checked` field does not
> makes sense on non checkbox type menu item. Can we just have `MenuItem` and
> `ToogleMenuItem` (or something with a better name). This also allows us to
> provide
> `MenuItem` on both mobile and desktop while providing `ToogleMenuItem` only
> on desktop (since we can't do it on mobile).

The latter point about not being able to do a Togglable menuitem on mobile is just wrong, see my other comments, you were looking at the simple MDN docs and not the java code.

I don't think that creating a whole new class is necessary.  We do this with the URL class for instance (take the host property for example).

Perhaps there is something more elegant we can do though, I'll leave this option out of the first round for now so we have more time to think it over.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #3)
> > 
> > It also maybe interesting to add `hotkey` property where `Hotkey` instance 
> > can be placed. If it is placed we could display the hotkey combination
> > similar to ones in built-in keys. This very much can be a future improvement
> > & definitely would be better to do in a second cut if ever.
> 
> How would you see this implemented Irakli? in bug 664361 you were against a
> xul based hotkey implementation which would be necessary here, I don't want
> to see two implementations of hotkeys in the sdk when it is unnecessary.  I
> think this functionality should be decoupled, and the hotkey module would
> make a hotkey that can be applied to UI components.  This way the UI
> component APIs are more easy to use, and implement.

I don't see how they are related to XUL, what we use under the hood to implement hotkeys is irrelevant. Our API allows us to see the combo that can be displayed along with label. And our hotkey listener can also just trigger action on the related menu item. Basically what I mean menu item will just dispaly `hotkey.combo` along with label and will set `hotkey.on("press", _ => emit(menuItem, "click"))`
Flags: needinfo?(rFobic)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #6)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #3)
> > 
> > # enabled
> > 
> > This is another case where there's no way to support this on Fennec at least
> > not right now. Do we really need it ?
> 
> 
> Again this is actually supported by Fennec
> https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/NativeWindow/
> menu/update
> 
> Not sure why you said it was not.  It is used by the 'Settings' menuitem,
> which is disabled until it is possible to view Settings.

I think docs have being updated since I commented :) Possibly because I was bugging fennec team last week about this stuff. Anyway feel free to discard this.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #3)
> > I would also add an icon field to menu item, although would only implement
> > support for icon on mobile since it does not really makes sense on desktop
> > and will cause
> > bad UX.
> 
> I'd like to leave icons out of the first round, I don't think it's very
> useful, and we can make that a follow up bug which would be a good first bug
> for someone else.

If you want to leave them out in first cut that's fine by me. Would still point out in first bug or JEP or somewhere that only local icons can be used.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #9)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #3)
> 
> > BTW if we end up doing high level API I think I'd prefer to have menu items
> > described in package.json.
> 
> Why?  that seems very anti-jetpack, as no other browser window UI component
> is required to be described in the package.json.  Also it seems painful and
> non-useful.  If you wanted to add a menuitem permission type, that seems
> more reasonable.

Declarative syntax is a lot simpler does not implies when those items have to be added or removed it's all up to the SDK decide when and how to do it. I also think this would cover all possible cases user gonna have without giving them too much flexibility, specially if we allow hiding those items. It also makes sense in the long term goal of add-on and b2g app synergy. Where you can define all of this in standard HTML markup in index.html and BTW there is a menu element standard http://www.w3.org/wiki/HTML/Elements/menu

However I do agree that sdk APIs have being different, but it's not a reason them to always stay the same and never improve. Either way I think we can move on with a current JEP and discuss higher level API (and weather this one is high enough) next week ;)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #10)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #3)
> > 
> > One thing that I would really want to change is the way we handle `type`.
> > Having it as an attribute does not really scales, meaning that more types
> > may have more
> > specific properties and have them around in the contexts where they don't
> > make
> > sense is kind of ugly. It's in fact already a case `checked` field does not
> > makes sense on non checkbox type menu item. Can we just have `MenuItem` and
> > `ToogleMenuItem` (or something with a better name). This also allows us to
> > provide
> > `MenuItem` on both mobile and desktop while providing `ToogleMenuItem` only
> > on desktop (since we can't do it on mobile).
> 
> The latter point about not being able to do a Togglable menuitem on mobile
> is just wrong, see my other comments, you were looking at the simple MDN
> docs and not the java code.
>

As I already pointed out I think doc's have being updated since I looked at it (or maybe I misread something). Never the less in a future we may have to support platforms that won't have Tooglable Menu items.

> 
> I don't think that creating a whole new class is necessary.  We do this with
> the URL class for instance (take the host property for example).
>

I don't think that's fare comparison & let me elaborate:

Types are very useful language construct and a lot of features in language and our modules are based on it:

if (item instanceof ToogleMenuItem)
  doThis()
else if (item instanceof SomethingElse)
  doThat()


doThis.define(ToogleMenuItem, ...)
doThis.define(MenuItem, ...)

Now what you essentially doing with `type` property is overloading meaning of type,
contradicting essential idea of the types in languages, that is they map to specific interface. If the interface starts to change based of specific attributes
like `type` in this case  whole notion of type to interface association is ruined.

More axis you have in a system more complex it gets since seeing patterns for
reasoning  is naturally harder. In this case `type` attribute does exactly that, adds another axis along with existing notion of Type / Class in a language to
control value's behavior / pattern.

Finally this approach does not scale, cause if we decide to introduce more types of
menu items, we won't just introduce new types we will grow interface of all existing
menu items. Type provided by language wouldn't have that issue.

> 
> Perhaps there is something more elegant we can do though, I'll leave this
> option out of the first round for now so we have more time to think it over.
>

Can you elaborate what is the problem with leveraging language features for expressing types because that's exactly what it is, in fact you even called
it type ?

I don't mind omitting this in first cut, but that would mean omitting toggleable
menu items and for no good reason IMO.
Alright Irakli you convinced me that I was wrong, I think you are right that a separate class makes more sense.  I'm not ToggableMenuitem is a good name though, since there are two toggable properties enabled and checked.  Maybe CheckableMenuItem instead?
Flags: needinfo?(rFobic)
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #14)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #9)
> > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > comment #3)
> > 
> > > BTW if we end up doing high level API I think I'd prefer to have menu items
> > > described in package.json.
> > 
> > Why?  that seems very anti-jetpack, as no other browser window UI component
> > is required to be described in the package.json.  Also it seems painful and
> > non-useful.  If you wanted to add a menuitem permission type, that seems
> > more reasonable.
> 
> Declarative syntax is a lot simpler does not implies when those items have
> to be added or removed it's all up to the SDK decide when and how to do it.
> I also think this would cover all possible cases user gonna have without
> giving them too much flexibility, specially if we allow hiding those items.
> It also makes sense in the long term goal of add-on and b2g app synergy.
> Where you can define all of this in standard HTML markup in index.html and
> BTW there is a menu element standard
> http://www.w3.org/wiki/HTML/Elements/menu

I don't understand any of this.  Do you want us to declare menu items in index.html now?  You previously stated that you wanted them declared in package.json.

I also don't see how this relates to the b2g app strategy.  Perhaps you can provide a link?
(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #11)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > comment #3)
> > > 
> > > It also maybe interesting to add `hotkey` property where `Hotkey` instance 
> > > can be placed. If it is placed we could display the hotkey combination
> > > similar to ones in built-in keys. This very much can be a future improvement
> > > & definitely would be better to do in a second cut if ever.
> > 
> > How would you see this implemented Irakli? in bug 664361 you were against a
> > xul based hotkey implementation which would be necessary here, I don't want
> > to see two implementations of hotkeys in the sdk when it is unnecessary.  I
> > think this functionality should be decoupled, and the hotkey module would
> > make a hotkey that can be applied to UI components.  This way the UI
> > component APIs are more easy to use, and implement.
> 
> I don't see how they are related to XUL, what we use under the hood to
> implement hotkeys is irrelevant. Our API allows us to see the combo that can
> be displayed along with label. And our hotkey listener can also just trigger
> action on the related menu item. Basically what I mean menu item will just
> dispaly `hotkey.combo` along with label and will set `hotkey.on("press", _
> => emit(menuItem, "click"))`

This doesn't make much sense to me.  As far as I can gather you want to make the label = label + hotkey.combo ??

If this is so then the menu item will look unprofessional, because the hotkey combo is supposted to be aligned to the right and the label is aligned to the left.  In xul this is done by associated a <menuitem> (see https://developer.mozilla.org/en-US/docs/XUL/menuitem ) to a <key> (see https://developer.mozilla.org/en-US/docs/XUL/key ) by using the menuitem using the key attribute (see https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-menuitem.key ) like so <menuitem key='key_id'/>

Which means we need the hotkey to be implemented with XUL.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #16)
> Alright Irakli you convinced me that I was wrong, I think you are right that
> a separate class makes more sense.  I'm not ToggableMenuitem is a good name
> though, since there are two toggable properties enabled and checked.  Maybe
> CheckableMenuItem instead?

Yeah I'm not good with naming I'll let native English speakers decide what naming makes more sense ;) I'd suggest maybe consult with Mossop or Jeff if you want more input on this. There's also maybe a specific name used by apple's or microsoft's for this type of menu items that we could borrow.
Flags: needinfo?(rFobic)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #14)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #9)
> > > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > > comment #3)
> > > 
> > > > BTW if we end up doing high level API I think I'd prefer to have menu items
> > > > described in package.json.
> > > 
> > > Why?  that seems very anti-jetpack, as no other browser window UI component
> > > is required to be described in the package.json.  Also it seems painful and
> > > non-useful.  If you wanted to add a menuitem permission type, that seems
> > > more reasonable.
> > 
> > Declarative syntax is a lot simpler does not implies when those items have
> > to be added or removed it's all up to the SDK decide when and how to do it.
> > I also think this would cover all possible cases user gonna have without
> > giving them too much flexibility, specially if we allow hiding those items.
> > It also makes sense in the long term goal of add-on and b2g app synergy.
> > Where you can define all of this in standard HTML markup in index.html and
> > BTW there is a menu element standard
> > http://www.w3.org/wiki/HTML/Elements/menu
> 
> I don't understand any of this.  Do you want us to declare menu items in
> index.html now?  You previously stated that you wanted them declared in
> package.json.
> 
> I also don't see how this relates to the b2g app strategy.  Perhaps you can
> provide a link?

How about we forget about this for a bit, since this does not blocks this work anyway. I'll try to give a broader picture where I see us going in long term
next week and we can talk about how this makes (or not) sense ;)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #17)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #14)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #9)
> > > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > > comment #3)
> > > 
> > > > BTW if we end up doing high level API I think I'd prefer to have menu items
> > > > described in package.json.
> > > 
> > > Why?  that seems very anti-jetpack, as no other browser window UI component
> > > is required to be described in the package.json.  Also it seems painful and
> > > non-useful.  If you wanted to add a menuitem permission type, that seems
> > > more reasonable.
> > 
> > Declarative syntax is a lot simpler does not implies when those items have
> > to be added or removed it's all up to the SDK decide when and how to do it.
> > I also think this would cover all possible cases user gonna have without
> > giving them too much flexibility, specially if we allow hiding those items.
> > It also makes sense in the long term goal of add-on and b2g app synergy.
> > Where you can define all of this in standard HTML markup in index.html and
> > BTW there is a menu element standard
> > http://www.w3.org/wiki/HTML/Elements/menu
> 
> I don't understand any of this.  Do you want us to declare menu items in
> index.html now?  You previously stated that you wanted them declared in
> package.json.
> 
> I also don't see how this relates to the b2g app strategy.  Perhaps you can
> provide a link?

How about we forget about this for a bit, since this does not blocks this work anyway. I'll try to give a broader picture where I see us going in long term
next week and we can talk about how this makes (or not) sense ;)(In reply to Erik Vold [:erikvold] [:ztatic] from comment #18)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #11)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> > > (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> > > comment #3)
> > > > 
> > > > It also maybe interesting to add `hotkey` property where `Hotkey` instance 
> > > > can be placed. If it is placed we could display the hotkey combination
> > > > similar to ones in built-in keys. This very much can be a future improvement
> > > > & definitely would be better to do in a second cut if ever.
> > > 
> > > How would you see this implemented Irakli? in bug 664361 you were against a
> > > xul based hotkey implementation which would be necessary here, I don't want
> > > to see two implementations of hotkeys in the sdk when it is unnecessary.  I
> > > think this functionality should be decoupled, and the hotkey module would
> > > make a hotkey that can be applied to UI components.  This way the UI
> > > component APIs are more easy to use, and implement.
> > 
> > I don't see how they are related to XUL, what we use under the hood to
> > implement hotkeys is irrelevant. Our API allows us to see the combo that can
> > be displayed along with label. And our hotkey listener can also just trigger
> > action on the related menu item. Basically what I mean menu item will just
> > dispaly `hotkey.combo` along with label and will set `hotkey.on("press", _
> > => emit(menuItem, "click"))`
> 
> This doesn't make much sense to me.  As far as I can gather you want to make
> the label = label + hotkey.combo ??

That's not what I meant, more details below.

> 
> If this is so then the menu item will look unprofessional, because the
> hotkey combo is supposted to be aligned to the right and the label is
> aligned to the left.  In xul this is done by associated a <menuitem> (see
> https://developer.mozilla.org/en-US/docs/XUL/menuitem ) to a <key> (see
> https://developer.mozilla.org/en-US/docs/XUL/key ) by using the menuitem
> using the key attribute (see
> https://developer.mozilla.org/en-US/docs/XUL/menuitem#a-menuitem.key ) like
> so <menuitem key='key_id'/>
> 
> Which means we need the hotkey to be implemented with XUL.

You are right Eirk, I was assuming that there's going to be a way to specify hotkey combo labels to the menuitems without actually creating keys. Looking at the docs it does not seem to be possible.

I have submitted Bug 907016 to add hotkeys support to menu items, we can discuss further how to get there in that bug. Either way this is definitely not something for the first cut so let's move on and forget about this for a time being.
Erik I think at this point we're in agreement. Would you mind to needinfo once JEP is up to date to make sure we're all in sync & we're ready to go.

P.S.: It looks like adding hotkey support will be easy after all (See comments in Bug 907016) so I'll let you decide weather you wanna include it in first cut or not.
JEP is updated.

I left out the tertiary class(es), whatever we decide to call it/them.
Flags: needinfo?(rFobic)
Attachment #793604 - Flags: review?(rFobic)
Erik I made some comments, in the pull.
Comment on attachment 793604 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1215

I made few comments in the pull r- until we're on the same page on those.
Attachment #793604 - Flags: review?(rFobic) → review-
Comment on attachment 793604 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1215

k I made the requested changes.
Attachment #793604 - Flags: review- → review?(rFobic)
Comment on attachment 793604 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1215

We're on the same page now, although I still made few remarks please see them before doing the implementation. If you do disagree with them please speak up and let's have another iteration, otherwise I'll assume you agree with them & if so I don't think we need more discussion on this.
Attachment #793604 - Flags: review?(rFobic) → review+
clear needinfo
Flags: needinfo?(rFobic)
I have some more questions for Irakli in the pull request.
Flags: needinfo?(rFobic)
Provided replies in pull request and discussed on IRC.
Flags: needinfo?(rFobic)
Attachment #818857 - Flags: review?(zer0)
Comment on attachment 818857 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1269

As mentioned with Irakli, I will assign this review to him: I don't want to slow down too many reviews, and I'm working on the Toolbar one at the moment, plus  Also, you two had already discussed a lot about this thing, so it would be probably easier.
Attachment #818857 - Flags: review?(zer0) → review?(rFobic)
Attachment #818857 - Flags: review?(rFobic) → review-
Comment on attachment 818857 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1269

Re-requesting a review, I addressed the fact that sdk/ui/menuitem/view was changing input, so that is no longer the case.
Attachment #818857 - Flags: review- → review?(rFobic)
Comment on attachment 793604 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1215

I think after session today we've agreed about implementation strategy. I'll remove review request, feel free to re-request once you thing it's ready.
Attachment #793604 - Flags: review+
Comment on attachment 793604 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1215

Oops changed a wrong one.
Attachment #793604 - Flags: review+
Depends on: 833011
So I got started on my 4th version of this module now and ran in to the issue that I mention here https://bugzilla.mozilla.org/show_bug.cgi?id=833011#c9 and since I have rewritten this so many times I would really rather wait to resolve bug 833011 before spending more time on this.
https://bugzilla.mozilla.org/show_bug.cgi?id=1399562
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: