Fix for Loop settings menu positioning where menu gets cropped from longer text

Comment on attachment 8665122 [details] [diff] [review] Attachment to Bug 1204101 - Gear menu cutoff with longer strings Review of attachment 8665122 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch; looks good; it just need a few tweaks. Please generally try and wrap things at 80 columns, except when it's likely to look particularly awkward not doing so. (Eg the comments here generally look re-wrappable). ::: browser/components/loop/content/shared/js/mixins.js @@ +230,5 @@ > > + // Added call to _repositionMenu() if it exists, to allow a component to add specific repositioning to a menu > + // This mixin should be deprecated and a new solution implemented for processing menus and taking care > + // of menu positioning globally. This new implementation should ensure all menus are positioned using the > + // same method of positioning I think this the last three lines of this comment want to go at the top of the mixin so that anyone looking at the mixin code for the first time will see it. It'd also be good to add a couple of sentences about what's wrong with the existing implementation, and a link to the bug where it will be replaced. @@ +231,5 @@ > + // Added call to _repositionMenu() if it exists, to allow a component to add specific repositioning to a menu > + // This mixin should be deprecated and a new solution implemented for processing menus and taking care > + // of menu positioning globally. This new implementation should ensure all menus are positioned using the > + // same method of positioning > + if(this._repositionMenu) { Nit: if(this._repositionMenu) needs a space between the if and the parens. If you could file a bug (and CC me) to add this rule <http://eslint.org/docs/rules/space-after-keywords> to our .eslintrc so that we can get a much tighter feedback loop for this problem, that'd be awesome. ::: browser/components/loop/content/shared/js/views.jsx @@ +202,5 @@ > }; > }, > > /** > + * Reposition Menu if cropped Does this work as-is in RTL mode? Or do we need to handle that side explicitly too. It'd be good to have a comment explaining exactly how it repositions it (i.e. it pulls it into the parent(?) just enough to be visible and no further).

Fix for Loop settings menu positioning where menu gets cropped from longer text

Comment on attachment 8665692 [details] [diff] [review] Attachment to Bug 1204101 - Gear menu cutoff with longer strings Review of attachment 8665692 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; r=dmose

Comment on attachment 8665692 [details] [diff] [review] Attachment to Bug 1204101 - Gear menu cutoff with longer strings Approval Request Comment [Feature/regressing bug #]: Hello visual refresh [User impact if declined]: If a localisation has long text for a string in a popup menu in the Hello conversation window, then the menu may be displayed with some of it outside of the window, but hidden, and hence not all of the text would be readable. [Describe test coverage new/current, TreeHerder]: Code is generally covered by tests. [Risks and why]: Low, small localised change to Hello's menu positioning code. [String/UUID change made/needed]: None

Comment on attachment 8665692 [details] [diff] [review] Attachment to Bug 1204101 - Gear menu cutoff with longer strings Minor change to Hello text and menu layout, OK to uplift to aurora