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]) {
diff --git a/polygerrit-ui/app/styles/shared-styles.html b/polygerrit-ui/app/styles/shared-styles.html index b241443..3a0de59 100644 --- a/polygerrit-ui/app/styles/shared-styles.html +++ b/polygerrit-ui/app/styles/shared-styles.html
@@ -162,6 +162,22 @@ strong { font-weight: var(--font-weight-bold); } + + /** BEGIN: loading spiner */ + .loadingSpin { + border: 2px solid var(--disabled-button-background-color); + border-top: 2px solid var(--primary-button-background-color); + border-radius: 50%; + width: 10px; + height: 10px; + animation: spin 2s linear infinite; + margin-right: var(--spacing-s); + } + @keyframes spin { + 0% { transform: rotate(0deg); } + 100% { transform: rotate(360deg); } + } + /** END: loading spiner */ </style> </template> </dom-module>