Skip to content
This repository has been archived by the owner on Dec 3, 2020. It is now read-only.

Fix #256: Change popup width in overflow menu #271

Merged
merged 3 commits into from
Nov 21, 2018
Merged

Fix #256: Change popup width in overflow menu #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a719bba
Fix #256: Change popup width in overflow menu
biancadanforth Nov 15, 2018
b6c508a
Detect overflow for explicit (un)pinning of toolbar button to overflo…
biancadanforth Nov 16, 2018
c60594f
Incorporate Osmose's feedback
biancadanforth Nov 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/browser_action/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ body {
}

#browser-action-app {
min-width: 320px;
max-width: 400px; /* Overflow menu popup width exceeds 320px and cannot be changed */
width: 320px;
}

#browser-action-app.overflow {
width: 100%;
}

Expand Down
6 changes: 6 additions & 0 deletions src/browser_action/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ if (extractedProductJSON) {
appProps.extractedProduct = JSON.parse(extractedProductJSON);
}

(async function checkOverflow() {
if (await browser.customizableUI.isWidgetInOverflow('shopping-testpilot_mozilla_org-browser-action')) {
document.getElementById('browser-action-app').classList.add('overflow');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, maybe we could put this into one of the React components, like in the state for BrowserActionApp? Not sure if that adds much to this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do this just now, but <BrowserActionApp /> currently returns a <React.Fragment>, which doesn't accept a class name. Wrapping the whole thing in a <div> instead and adding the class to that is possible, but then the CSS also needs another layer instead of applying the overflow class directly to #browser-action-app:

#browser-action-app > div {
  width: 320px;
}

#browser-action-app > div.overflow {
  width: 100%;
}

Also with the new Study Invitation screen (#274 ), I'd need that wrapper div in two places in BrowserActionApp instead of just one.

I also tried putting the class name directly on <BrowserActionApp /> in index.jsx, but ReactDOM.render didn't like the await keyword needed for browser.customizableUI.isWidgetInOverflow.

I think for simplicity, I'll just leave this as is for now. Perhaps we can refactor this later if we ever implement the view router pattern you suggested here... The options I can think of/try right now seem more confusing and more prone to bugs.

}
}());

ReactDOM.render(
<Provider store={store}>
<BrowserActionApp {...appProps} />
Expand Down
16 changes: 13 additions & 3 deletions src/experiment_apis/customizableUI/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
/* global ChromeUtils ExtensionAPI ExtensionCommon */
/* global ChromeUtils ExtensionAPI */

this.customizableUI = class extends ExtensionAPI {
getAPI(context) {
ChromeUtils.import('resource://gre/modules/ExtensionCommon.jsm');
const {CustomizableUI} = ChromeUtils.import('resource:///modules/CustomizableUI.jsm');
const {ExtensionCommon} = ChromeUtils.import('resource://gre/modules/ExtensionCommon.jsm');
const {EventManager} = ExtensionCommon;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of this PR, but this seems weirdly formatted compared to the other imports here.

const {CustomizableUI} = ChromeUtils.import('resource:///modules/CustomizableUI.jsm', {});
const {Services} = ChromeUtils.import('resource://gre/modules/Services.jsm');
return {
customizableUI: {
onWidgetRemoved: new EventManager(
Expand All @@ -25,6 +26,15 @@ this.customizableUI = class extends ExtensionAPI {
};
},
).api(),
async isWidgetInOverflow(widgetId) {
const {area} = CustomizableUI.getPlacementOfWidget(widgetId);
const browserWindow = Services.wm.getMostRecentWindow('navigator:browser');
// First check is for the non-fixed overflow menu (e.g. widget moved by resizing window)
// Second is for fixed overflow menu (e.g. widget moved by (un)pinning button to overflow)
return (
CustomizableUI.getWidget(widgetId).forWindow(browserWindow).overflowed
|| area === 'widget-overflow-fixed-list');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This becomes slightly more readable IMO if we format this like this:

return (
  CustomizableUI.getWidget(widgetId).forWindow(browserWindow).overflowed
  || area === 'widget-overflow-fixed-list'
);

},
},
};
}
Expand Down
17 changes: 16 additions & 1 deletion src/experiment_apis/customizableUI/schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
{
"name": "onWidgetRemoved",
"type": "function",
"description": "Fired when a widget is removed from the browser chrome",
"description": "Fired when a widget is removed from the browser chrome.",
"parameters": [
{
"name": "widgetId",
Expand All @@ -14,6 +14,21 @@
}
]
}
],
"functions": [
{
"name": "isWidgetInOverflow",
"type": "function",
"description": "Determine whether or not a widget is in the overflow menu.",
"async": true,
"parameters": [
{
"name": "widgetId",
"type": "string",
"description": "The id of the widget to check."
}
]
}
]
}
]