Skip to content

Commit

Permalink
[FilesTrash] Turn learnMoreLink into extraButton in progress panel
Browse files Browse the repository at this point in the history
The learnMoreLink that is used for the DriveFS pooled storage is useful
as a way to have an additional button in the visual signals. This
updates the learnMoreLink to be more generic enabling an arbitrary
callback to be supplied.

Bug: b:243879041
Test: CL:3879043
Test: browser_tests --gtest_filter=FileManagerJsTest.FilesDisplayPanel
Test: tast run filemanager.DrivefsPooledStorage.full_individual
Change-Id: I173d3b0d40cc7c0ed5499febb1bc4d11deb2a56b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3876771
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Ben Reich <benreich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1044409}
  • Loading branch information
ben-reich authored and Chromium LUCI CQ committed Sep 8, 2022
1 parent 9cbf069 commit 0c18d8f
Show file tree
Hide file tree
Showing 8 changed files with 149 additions and 26 deletions.
10 changes: 7 additions & 3 deletions ui/file_manager/file_manager/background/js/drive_sync_handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {NativeEventTarget as EventTarget} from 'chrome://resources/js/cr/event_t
import {AsyncUtil} from '../../common/js/async_util.js';
import {ProgressCenterItem, ProgressItemState, ProgressItemType} from '../../common/js/progress_center_common.js';
import {getFilesAppIconURL, toFilesAppURL} from '../../common/js/url_constants.js';
import {str, strf} from '../../common/js/util.js';
import {str, strf, util} from '../../common/js/util.js';
import {xfm} from '../../common/js/xfm.js';
import {DriveSyncHandler} from '../../externs/background/drive_sync_handler.js';
import {ProgressCenter} from '../../externs/background/progress_center.js';
Expand Down Expand Up @@ -356,7 +356,9 @@ export class DriveSyncHandlerImpl extends EventTarget {
break;
case 'no_server_space':
item.message = strf('SYNC_NO_SERVER_SPACE');
item.learnMoreLink = str('GOOGLE_DRIVE_MANAGE_STORAGE_URL');
item.setExtraButton(
ProgressItemState.ERROR, str('LEARN_MORE_LABEL'),
() => util.visitURL(str('GOOGLE_DRIVE_MANAGE_STORAGE_URL')));

// This error will reappear every time sync is retried, so we use
// a fixed ID to avoid spamming the user.
Expand All @@ -365,7 +367,9 @@ export class DriveSyncHandlerImpl extends EventTarget {
break;
case 'no_server_space_organization':
item.message = strf('SYNC_NO_SERVER_SPACE_ORGANIZATION');
item.learnMoreLink = str('GOOGLE_DRIVE_MANAGE_STORAGE_URL');
item.setExtraButton(
ProgressItemState.ERROR, str('LEARN_MORE_LABEL'),
() => util.visitURL(str('GOOGLE_DRIVE_MANAGE_STORAGE_URL')));

// This error will reappear every time sync is retried, so we use
// a fixed ID to avoid spamming the user.
Expand Down
42 changes: 38 additions & 4 deletions ui/file_manager/file_manager/common/js/progress_center_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,17 @@ export const ProgressItemType = {
};
Object.freeze(ProgressItemType);

/**
* Visual signals can have an additional button that, when clicked, performs
* some arbitrary action. The `text` defines the button text to show and the
* `callback` defines the arbitrary action.
* @typedef {{
* text: string,
* callback: !function(),
* }}
*/
export let ProgressItemExtraButton;

/**
* Item of the progress center.
*/
Expand Down Expand Up @@ -135,11 +146,34 @@ export class ProgressCenterItem {
this.remainingTime;

/**
* Link to be opened when users click the "Learn more" button.
* The "Learn more" button won't be displayed if this is falsy.
* @type {?string}
* Contains the text and callback on an extra button when the progress
* center item is either in COMPLETED or ERROR state.
* @type {!Map<!ProgressItemState, !ProgressItemExtraButton>}
*/
this.learnMoreLink;
this.extraButton = new Map();
}

/**
* Sets the extra button text and callback. Use this to add an additional
* button with configurable functionality.
* @param {string} text Text to use for the button.
* @param {!ProgressItemState} state Which state to show the button,
* currently only `ProgressItemState.COMPLETED` and
* `ProgressItemState.ERROR` are supported.
* @param {!function()} callback The callback to invoke when the button is
* pressed.
*/
setExtraButton(state, text, callback) {
if (!text || !callback) {
console.warn('Text and callback must be supplied');
return;
}
if (this.extraButton.has(state)) {
console.warn('Extra button already defined for state:', state);
return;
}
const extraButton = {text, callback};
this.extraButton.set(state, extraButton);
}

/**
Expand Down
1 change: 1 addition & 0 deletions ui/file_manager/file_manager/foreground/elements/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ js_unittest("files_xf_elements_unittest") {
deps = [
":xf_display_panel",
"//chrome/test/data/webui:chai_assert",
"//ui/file_manager/file_manager/common/js:test_error_reporting",
"//ui/file_manager/file_manager/common/js:util",
"//ui/webui/resources/js:assert.m",
"//ui/webui/resources/js:load_time_data.m",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import {assert} from 'chrome://resources/js/assert.m.js';
import {assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';

import {waitUntil} from '../../common/js/test_error_reporting.js';
import {util} from '../../common/js/util.js';

import {DisplayPanel} from './xf_display_panel.js';
Expand Down Expand Up @@ -142,7 +143,7 @@ export async function testDisplayPanelChangingPanelTypes(done) {

// Check dismiss signal from the panel from a click.
/** @type {!HTMLElement} */
let dismiss = assert(panelItem.secondaryButton);
let dismiss = assert(panelItem.primaryButton);
dismiss.click();
assertEquals(
signal, 'dismiss', 'Expected signal name "dismiss". Got ' + signal);
Expand Down Expand Up @@ -488,3 +489,43 @@ export async function testFilesDisplayPanelTransferDetailsSummary(done) {

done();
}

/**
* Tests that the extra-button appears when the value is in the dataset and
* sends the appropriate signal on click.
*/
export async function testExtraButtonCanBeAddedAndRespondsToAction(done) {
// Get the host display panel container element.
/** @type {!DisplayPanel|!Element} */
const displayPanel = assert(document.querySelector('#test-xf-display-panel'));
const panelItem = displayPanel.addPanelItem('testpanel');
panelItem.dataset.extraButtonText = 'Extra button';
panelItem.panelType = panelItem.panelTypeDone;

// Setup the panel item signal (click) callback.
/** @type {?string} */
let signal = null;
panelItem.signalCallback = (name) => {
assert(typeof name === 'string');
signal = name;
};

/** @type {!HTMLElement} */
const extraButton = assert(panelItem.primaryButton);
extraButton.click();
await waitUntil(() => !!signal);
assertEquals(
signal, 'extra-button',
'Expected signal name "extra-button". Got ' + signal);
signal = null;

// Check dismiss signal from the panel from a click.
/** @type {!HTMLElement} */
const dismiss = assert(panelItem.secondaryButton);
dismiss.click();
await waitUntil(() => !!signal);
assertEquals(
signal, 'dismiss', 'Expected signal name "dismiss". Got ' + signal);

done();
}
10 changes: 5 additions & 5 deletions ui/file_manager/file_manager/foreground/elements/xf_button.html
Original file line number Diff line number Diff line change
Expand Up @@ -57,34 +57,34 @@
position: relative;
}

:host(:not([data-category='dismiss']):not([data-category='learn-more'])) {
:host(:not([data-category='dismiss']):not([data-category='extra-button'])) {
width: 36px;
}

:host([data-category='dismiss']) #icon {
display: none;
}

:host([data-category='learn-more']) #icon {
:host([data-category='extra-button']) #icon {
display: none;
}

#dismiss {
display: none;
}

#learn-more {
#extra-button {
display: none;
}

:host([data-category='dismiss']) #dismiss {
display: unset;
}

:host([data-category='learn-more']) #learn-more {
:host([data-category='extra-button']) #extra-button {
display: unset;
}
</style>
<cr-button id='dismiss'>$i18n{DRIVE_WELCOME_DISMISS}</cr-button>
<cr-button id='learn-more'>$i18n{LEARN_MORE_LABEL}</cr-button>
<cr-button id='extra-button'></cr-button>
<cr-icon-button id='icon'></cr-icon-button>
9 changes: 9 additions & 0 deletions ui/file_manager/file_manager/foreground/elements/xf_button.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 21 additions & 8 deletions ui/file_manager/file_manager/foreground/elements/xf_panel_item.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,10 @@ export class ProgressCenterPanel {
panelItem.primaryText = primaryText;
panelItem.setAttribute('data-progress-id', item.id);

// Certain visual signals have the functionality to display an extra
// button with an arbitrary callback.
let extraButton = null;

// On progress panels, make the cancel button aria-label more useful.
const cancelLabel = strf('CANCEL_ACTIVITY_LABEL', primaryText);
panelItem.closeButtonAriaLabel = cancelLabel;
Expand All @@ -325,8 +329,13 @@ export class ProgressCenterPanel {
} else if (signal === 'dismiss') {
this.feedbackHost_.removePanelItem(panelItem);
this.dismissErrorItemCallback(item.id);
} else if (signal === 'learn-more') {
window.open(item.learnMoreLink, '_blank');
} else if (
signal === 'extra-button' && extraButton && extraButton.callback) {
extraButton.callback();
this.feedbackHost_.removePanelItem(panelItem);
// The extra-button currently acts as a dismissal to invoke the
// error item callback as well.
this.dismissErrorItemCallback(item.id);
}
};
panelItem.progress = item.progressRateInPercent.toString();
Expand All @@ -339,8 +348,13 @@ export class ProgressCenterPanel {
item.type === ProgressItemType.FORMAT ||
item.type === ProgressItemType.ZIP ||
item.type === ProgressItemType.DELETE ||
item.type == ProgressItemType.RESTORE_TO_DESTINATION) {
item.type === ProgressItemType.TRASH ||
item.type === ProgressItemType.RESTORE_TO_DESTINATION) {
const donePanelItem = this.feedbackHost_.addPanelItem(item.id);
if (item.extraButton.has(ProgressItemState.COMPLETED)) {
extraButton = item.extraButton.get(ProgressItemState.COMPLETED);
donePanelItem.dataset.extraButtonText = extraButton.text;
}
donePanelItem.id = item.id;
donePanelItem.panelType = donePanelItem.panelTypeDone;
donePanelItem.primaryText = primaryText;
Expand All @@ -349,6 +363,12 @@ export class ProgressCenterPanel {
if (signal === 'dismiss') {
this.feedbackHost_.removePanelItem(donePanelItem);
delete this.items_[donePanelItem.id];
} else if (
signal === 'extra-button' && extraButton &&
extraButton.callback) {
extraButton.callback();
this.feedbackHost_.removePanelItem(donePanelItem);
delete this.items_[donePanelItem.id];
}
};
// Delete after 4 seconds, doesn't matter if it's manually deleted
Expand All @@ -364,8 +384,9 @@ export class ProgressCenterPanel {
this.feedbackHost_.removePanelItem(panelItem);
break;
case ProgressItemState.ERROR:
if (item.learnMoreLink) {
panelItem.dataset.learnMoreLink = item.learnMoreLink;
if (item.extraButton.has(ProgressItemState.ERROR)) {
extraButton = item.extraButton.get(ProgressItemState.ERROR);
panelItem.dataset.extraButtonText = extraButton.text;
}
panelItem.panelType = panelItem.panelTypeError;
panelItem.primaryText = item.message;
Expand Down

0 comments on commit 0c18d8f

Please sign in to comment.