Support loading for button and allow event listener to set it
currently gr-button supports `loading` but did not use it, this change will:
1. use disabled status when loading
2. add a spinner to show when loading is true
3. in repo-access component, set loading to true while promise ongoing
4. also fix the issue with non-zero initial tabindex for gr-button
screenshot: https://imgur.com/a/NCLwb45
Bug: 12421
Change-Id: Ie69c447455d23a118dbe1823c576144e0bca7c2d
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html
index fab730d..54006b5 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html
+++ b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.html
@@ -105,12 +105,12 @@
primary
class$="[[_computeSaveBtnClass(_ownerOf)]]"
on-click="_handleSave"
- disabled$="[[!_modified]]">Save</gr-button>
+ disabled="[[!_modified]]">Save</gr-button>
<gr-button id="saveReviewBtn"
primary
class$="[[_computeSaveReviewBtnClass(_canUpload)]]"
on-click="_handleSaveForReview"
- disabled$="[[!_modified]]">Save for review</gr-button>
+ disabled="[[!_modified]]">Save for review</gr-button>
<template
is="dom-repeat"
items="{{_sections}}"
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js
index b350702..02b62e0 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js
+++ b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access.js
@@ -442,21 +442,42 @@
return obj;
}
- _handleSave() {
+ _handleSave(e) {
const obj = this._getObjforSave();
if (!obj) { return; }
+ const button = e && e.target;
+ if (button) {
+ button.loading = true;
+ }
return this.$.restAPI.setRepoAccessRights(this.repo, obj)
.then(() => {
this._reload(this.repo);
+ })
+ .finally(() => {
+ this._modified = false;
+ if (button) {
+ button.loading = false;
+ }
});
}
- _handleSaveForReview() {
+ _handleSaveForReview(e) {
const obj = this._getObjforSave();
if (!obj) { return; }
- return this.$.restAPI.setRepoAccessRightsForReview(this.repo, obj)
+ const button = e && e.target;
+ if (button) {
+ button.loading = true;
+ }
+ return this.$.restAPI
+ .setRepoAccessRightsForReview(this.repo, obj)
.then(change => {
Gerrit.Nav.navigateToChange(change);
+ })
+ .finally(() => {
+ this._modified = false;
+ if (button) {
+ button.loading = false;
+ }
});
}
diff --git a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html
index 2fc31f7..d89b5df 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html
+++ b/polygerrit-ui/app/elements/admin/gr-repo-access/gr-repo-access_test.html
@@ -1138,6 +1138,53 @@
assert.equal(Object.keys(element._local).length, 1);
});
+ test('_handleSave', done => {
+ const repoAccessInput = {
+ add: {
+ 'refs/*': {
+ permissions: {
+ owner: {
+ rules: {
+ 123: {action: 'DENY', modified: true},
+ },
+ },
+ },
+ },
+ },
+ remove: {
+ 'refs/*': {
+ permissions: {
+ owner: {
+ rules: {
+ 123: {},
+ },
+ },
+ },
+ },
+ },
+ };
+ sandbox.stub(element.$.restAPI, 'getRepoAccessRights').returns(
+ Promise.resolve(JSON.parse(JSON.stringify(accessRes))));
+ sandbox.stub(Gerrit.Nav, 'navigateToChange');
+ let resolver;
+ const saveStub = sandbox.stub(element.$.restAPI,
+ 'setRepoAccessRights')
+ .returns(new Promise(r => resolver = r));
+
+ element.repo = 'test-repo';
+ sandbox.stub(element, '_computeAddAndRemove').returns(repoAccessInput);
+
+ element._modified = true;
+ MockInteractions.tap(element.$.saveBtn);
+ assert.equal(element.$.saveBtn.hasAttribute('loading'), true);
+ resolver({_number: 1});
+ flush(() => {
+ assert.isTrue(saveStub.called);
+ assert.isTrue(Gerrit.Nav.navigateToChange.notCalled);
+ done();
+ });
+ });
+
test('_handleSaveForReview', done => {
const repoAccessInput = {
add: {
@@ -1166,14 +1213,19 @@
sandbox.stub(element.$.restAPI, 'getRepoAccessRights').returns(
Promise.resolve(JSON.parse(JSON.stringify(accessRes))));
sandbox.stub(Gerrit.Nav, 'navigateToChange');
+ let resolver;
const saveForReviewStub = sandbox.stub(element.$.restAPI,
'setRepoAccessRightsForReview')
- .returns(Promise.resolve({_number: 1}));
+ .returns(new Promise(r => resolver = r));
element.repo = 'test-repo';
sandbox.stub(element, '_computeAddAndRemove').returns(repoAccessInput);
- element._handleSaveForReview().then(() => {
+ element._modified = true;
+ MockInteractions.tap(element.$.saveReviewBtn);
+ assert.equal(element.$.saveReviewBtn.hasAttribute('loading'), true);
+ resolver({_number: 1});
+ flush(() => {
assert.isTrue(saveForReviewStub.called);
assert.isTrue(Gerrit.Nav.navigateToChange
.lastCall.calledWithExactly({_number: 1}));
diff --git a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
index 498bf6b..729f87d 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.html
@@ -137,7 +137,7 @@
/* Keep below color definition for primary so that this takes precedence
when disabled. */
- :host([disabled]) {
+ :host([disabled]), :host([loading]) {
--background-color: var(--disabled-button-background-color);
--text-color: var(--deemphasized-text-color);
cursor: default;
@@ -149,7 +149,8 @@
--margin: 0;
--padding: 5px 4px;
}
- :host([disabled][link]) {
+ :host([disabled][link]),
+ :host([loading][link]) {
--background-color: transparent;
--text-color: var(--deemphasized-text-color);
cursor: default;
@@ -173,8 +174,11 @@
</style>
<paper-button
raised="[[!link]]"
- disabled="[[_computeDisabled(disabled, loading)]]"
+ disabled="[[_disabled]]"
tabindex="-1">
+ <template is="dom-if" if="[[loading]]">
+ <span class="loadingSpin"></span>
+ </template>
<slot></slot>
<i class="downArrow"></i>
</paper-button>
diff --git a/polygerrit-ui/app/elements/shared/gr-button/gr-button.js b/polygerrit-ui/app/elements/shared/gr-button/gr-button.js
index a078be7..9d96038 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button.js
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button.js
@@ -42,11 +42,6 @@
value: false,
reflectToAttribute: true,
},
- loading: {
- type: Boolean,
- value: false,
- reflectToAttribute: true,
- },
disabled: {
type: Boolean,
observer: '_disabledChanged',
@@ -56,24 +51,29 @@
type: Boolean,
value: false,
},
- _enabledTabindex: {
+ loading: {
+ type: Boolean,
+ value: false,
+ reflectToAttribute: true,
+ },
+
+ _disabled: {
+ type: Boolean,
+ computed: '_computeDisabled(disabled, loading)',
+ },
+
+ _initialTabindex: {
type: String,
value: '0',
},
};
}
- static get observers() {
- return [
- '_computeDisabled(disabled, loading)',
- ];
- }
-
/** @override */
created() {
super.created();
- this.addEventListener('click',
- e => this._handleAction(e));
+ this._initialTabindex = this.getAttribute('tabindex') || '0';
+ this.addEventListener('click', e => this._handleAction(e));
this.addEventListener('keydown',
e => this._handleKeydown(e));
}
@@ -86,10 +86,13 @@
}
_handleAction(e) {
- if (this.disabled) {
+ if (this._disabled) {
e.preventDefault();
+ e.stopPropagation();
e.stopImmediatePropagation();
+ return;
}
+
let el = this.root;
let path = '';
while (el = el.parentNode || el.host) {
@@ -106,10 +109,7 @@
}
_disabledChanged(disabled) {
- if (disabled) {
- this._enabledTabindex = this.getAttribute('tabindex') || '0';
- }
- this.setAttribute('tabindex', disabled ? '-1' : this._enabledTabindex);
+ this.setAttribute('tabindex', disabled ? '-1' : this._initialTabindex);
this.updateStyles();
}
diff --git a/polygerrit-ui/app/elements/shared/gr-button/gr-button_test.html b/polygerrit-ui/app/elements/shared/gr-button/gr-button_test.html
index 0aec751..cfac37f 100644
--- a/polygerrit-ui/app/elements/shared/gr-button/gr-button_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-button/gr-button_test.html
@@ -35,8 +35,14 @@
</template>
</test-fixture>
+<test-fixture id="tabindex">
+ <template>
+ <gr-button tabindex="3"></gr-button>
+ </template>
+</test-fixture>
+
<script>
- suite('gr-select tests', async () => {
+ suite('gr-button tests', async () => {
await readyToTest();
let element;
let sandbox;
@@ -60,18 +66,32 @@
sandbox.restore();
});
- test('disabled is set by disabled or loading', () => {
- assert.isFalse(element.shadowRoot
- .querySelector('paper-button').disabled);
+ test('disabled is set by disabled', () => {
+ const paperBtn = element.shadowRoot.querySelector('paper-button');
+ assert.isFalse(paperBtn.disabled);
element.disabled = true;
- assert.isTrue(element.shadowRoot
- .querySelector('paper-button').disabled);
+ assert.isTrue(paperBtn.disabled);
element.disabled = false;
- assert.isFalse(element.shadowRoot
- .querySelector('paper-button').disabled);
- element.loading = true;
- assert.isTrue(element.shadowRoot
- .querySelector('paper-button').disabled);
+ assert.isFalse(paperBtn.disabled);
+ });
+
+ test('loading set from listener', done => {
+ let resolve;
+ element.addEventListener('click', e => {
+ e.target.loading = true;
+ resolve = () => e.target.loading = false;
+ });
+ const paperBtn = element.shadowRoot.querySelector('paper-button');
+ assert.isFalse(paperBtn.disabled);
+ MockInteractions.tap(element);
+ assert.isTrue(paperBtn.disabled);
+ assert.isTrue(element.hasAttribute('loading'));
+ resolve();
+ flush(() => {
+ assert.isFalse(paperBtn.disabled);
+ assert.isFalse(element.hasAttribute('loading'));
+ done();
+ });
});
test('tabindex should be -1 if disabled', () => {
@@ -82,22 +102,42 @@
// Regression tests for Issue: 11969
test('tabindex should be reset to 0 if enabled', () => {
element.disabled = false;
- assert.isTrue(element.getAttribute('tabindex') === '0');
+ assert.equal(element.getAttribute('tabindex'), '0');
element.disabled = true;
- assert.isTrue(element.getAttribute('tabindex') === '-1');
+ assert.equal(element.getAttribute('tabindex'), '-1');
element.disabled = false;
- assert.isTrue(element.getAttribute('tabindex') === '0');
+ assert.equal(element.getAttribute('tabindex'), '0');
+ });
+
+ test('tabindex should be preserved', () => {
+ element = fixture('tabindex');
+ element.disabled = false;
+ assert.equal(element.getAttribute('tabindex'), '3');
+ element.disabled = true;
+ assert.equal(element.getAttribute('tabindex'), '-1');
+ element.disabled = false;
+ assert.equal(element.getAttribute('tabindex'), '3');
});
// 'tap' event is tested so we don't loose backward compatibility with older
// plugins who didn't move to on-click which is faster and well supported.
- for (const eventName of ['tap', 'click']) {
- test('dispatches ' + eventName + ' event', () => {
- const spy = addSpyOn(eventName);
- MockInteractions.tap(element);
- assert.isTrue(spy.calledOnce);
- });
- }
+ test('dispatches click event', () => {
+ const spy = addSpyOn('click');
+ MockInteractions.click(element);
+ assert.isTrue(spy.calledOnce);
+ });
+
+ test('dispatches tap event', () => {
+ const spy = addSpyOn('tap');
+ MockInteractions.tap(element);
+ assert.isTrue(spy.calledOnce);
+ });
+
+ test('dispatches click from tap event', () => {
+ const spy = addSpyOn('click');
+ MockInteractions.tap(element);
+ assert.isTrue(spy.calledOnce);
+ });
// Keycodes: 32 for Space, 13 for Enter.
for (const key of [32, 13]) {
