From 5585a781b9e1e090b8e01301150d19f339ae4708 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 12 Dec 2019 13:35:52 +0000 Subject: [PATCH] Unified tag and member screen code no issue The tag and member screens share the same underlying UI/UX patterns but were using different code patterns. This brings both in line so that we have consistent code patterns that can be re-used for other screens. - fixed cleanup of new tags by adding the `deactivate` hook to the `tag` route - updated `member` and `member.new` route/controller setup to match tag route/controller setup - added `save` action to member controller so that Ctrl/Cmd+S works on member screen - updated tag route/controller to utilise the same instant display w/background refresh when accessing the tag details screen - completed transition of non-component tag/members templates over to angle bracket component syntax --- ghost/admin/app/controllers/member.js | 34 +++---- ghost/admin/app/controllers/member/new.js | 104 ---------------------- ghost/admin/app/controllers/tag.js | 32 ++++--- ghost/admin/app/routes/member.js | 36 ++++---- ghost/admin/app/routes/member/new.js | 47 +--------- ghost/admin/app/routes/tag.js | 28 +++++- ghost/admin/app/templates/member.hbs | 84 ++++++++++------- ghost/admin/app/templates/member/new.hbs | 67 -------------- ghost/admin/app/templates/members.hbs | 78 ++++++++-------- ghost/admin/app/templates/tag.hbs | 36 ++++---- ghost/admin/app/templates/tags.hbs | 33 +++---- 11 files changed, 205 insertions(+), 374 deletions(-) delete mode 100644 ghost/admin/app/controllers/member/new.js delete mode 100644 ghost/admin/app/templates/member/new.hbs diff --git a/ghost/admin/app/controllers/member.js b/ghost/admin/app/controllers/member.js index 9a7d0da2d4..cb2e36dbe1 100644 --- a/ghost/admin/app/controllers/member.js +++ b/ghost/admin/app/controllers/member.js @@ -8,13 +8,12 @@ import {task} from 'ember-concurrency'; export default Controller.extend({ members: controller(), + notifications: service(), + router: service(), store: service(), - router: service(), - - notifications: service(), - member: alias('model'), + subscribedAt: computed('member.createdAtUTC', function () { let memberSince = moment(this.member.createdAtUTC).from(moment()); let createdDate = moment(this.member.createdAtUTC).format('MMM DD, YYYY'); @@ -25,9 +24,15 @@ export default Controller.extend({ setProperty(propKey, value) { this._saveMemberProperty(propKey, value); }, + toggleDeleteMemberModal() { this.toggleProperty('showDeleteMemberModal'); }, + + save() { + return this.save.perform(); + }, + finaliseDeletion() { // decrement the total member count manually so there's no flash // when transitioning back to the members list @@ -62,21 +67,8 @@ export default Controller.extend({ }, leaveScreen() { - let transition = this.leaveScreenTransition; - - if (!transition) { - this.notifications.showAlert('Sorry, there was an error in the application. Please let the Ghost team know what happened.', {type: 'error'}); - return; - } - - // roll back changes on model props this.member.rollbackAttributes(); - - return transition.retry(); - }, - - save() { - return this.save.perform(); + return this.leaveScreenTransition.retry(); } }, @@ -98,11 +90,13 @@ export default Controller.extend({ fetchMember: task(function* (memberId) { this.set('isLoading', true); + yield this.store.findRecord('member', memberId, { reload: true - }).then((data) => { - this.set('member', data); + }).then((member) => { + this.set('member', member); this.set('isLoading', false); + return member; }); }) diff --git a/ghost/admin/app/controllers/member/new.js b/ghost/admin/app/controllers/member/new.js deleted file mode 100644 index 6eb1e66719..0000000000 --- a/ghost/admin/app/controllers/member/new.js +++ /dev/null @@ -1,104 +0,0 @@ -import Controller from '@ember/controller'; -import moment from 'moment'; -import {alias} from '@ember/object/computed'; -import {computed} from '@ember/object'; -import {inject as controller} from '@ember/controller'; -import {inject as service} from '@ember/service'; -import {task} from 'ember-concurrency'; - -export default Controller.extend({ - members: controller(), - store: service(), - - router: service(), - - notifications: service(), - - member: alias('model'), - - displayName: computed('member.{name,email}', function () { - return this.member.name || this.member.email || 'New member'; - }), - subscribedAt: computed('member.createdAtUTC', function () { - let memberSince = moment(this.member.createdAtUTC).from(moment()); - let createdDate = moment(this.member.createdAtUTC).format('MMM DD, YYYY'); - return `${createdDate} (${memberSince})`; - }), - - actions: { - setProperty(propKey, value) { - this._saveMemberProperty(propKey, value); - }, - - toggleDeleteMemberModal() { - this.toggleProperty('showDeleteMemberModal'); - }, - - finaliseDeletion() { - // decrement the total member count manually so there's no flash - // when transitioning back to the members list - if (this.members.memberCount) { - this.members.decrementProperty('memberCount'); - } - this.router.transitionTo('members'); - }, - - toggleUnsavedChangesModal(transition) { - let leaveTransition = this.leaveScreenTransition; - - if (!transition && this.showUnsavedChangesModal) { - this.set('leaveScreenTransition', null); - this.set('showUnsavedChangesModal', false); - return; - } - - if (!leaveTransition || transition.targetName === leaveTransition.targetName) { - this.set('leaveScreenTransition', transition); - - // if a save is running, wait for it to finish then transition - if (this.save.isRunning) { - return this.save.last.then(() => { - transition.retry(); - }); - } - - // we genuinely have unsaved data, show the modal - this.set('showUnsavedChangesModal', true); - } - }, - - leaveScreen() { - let transition = this.leaveScreenTransition; - - if (!transition) { - this.notifications.showAlert('Sorry, there was an error in the application. Please let the Ghost team know what happened.', {type: 'error'}); - return; - } - - // roll back changes on model props - this.member.rollbackAttributes(); - - return transition.retry(); - }, - - save() { - return this.save.perform(); - } - }, - - save: task(function* () { - let member = this.member; - try { - return yield member.save(); - } catch (error) { - if (error) { - this.notifications.showAPIError(error, {key: 'member.save'}); - } - } - }).drop(), - - _saveMemberProperty(propKey, newValue) { - let member = this.member; - member.set(propKey, newValue); - } -}); diff --git a/ghost/admin/app/controllers/tag.js b/ghost/admin/app/controllers/tag.js index 03af43dfeb..9c74d71c3d 100644 --- a/ghost/admin/app/controllers/tag.js +++ b/ghost/admin/app/controllers/tag.js @@ -23,7 +23,11 @@ export default Controller.extend({ }, deleteTag() { - return this._deleteTag(); + return this.tag.destroyRecord().then(() => { + return this.transitionToRoute('tags'); + }, (error) => { + return this.notifications.showAPIError(error, {key: 'tag.delete'}); + }); }, save() { @@ -55,17 +59,8 @@ export default Controller.extend({ }, leaveScreen() { - let transition = this.leaveScreenTransition; - - if (!transition) { - this.notifications.showAlert('Sorry, there was an error in the application. Please let the Ghost team know what happened.', {type: 'error'}); - return; - } - - // roll back changes on model props this.tag.rollbackAttributes(); - - return transition.retry(); + return this.leaveScreenTransition.retry(); } }, @@ -93,6 +88,7 @@ export default Controller.extend({ } tag.set('slug', slugValue); } + // TODO: This is required until .validate/.save mark fields as validated tag.get('hasValidated').addObject(propKey); }, @@ -125,11 +121,13 @@ export default Controller.extend({ } }), - _deleteTag() { - return this.tag.destroyRecord().then(() => { - return this.transitionToRoute('tags'); - }, (error) => { - return this.notifications.showAPIError(error, {key: 'tag.delete'}); + fetchTag: task(function* (slug) { + this.set('isLoading', true); + + yield this.store.queryRecord('tag', {slug}).then((tag) => { + this.set('tag', tag); + this.set('isLoading', false); + return tag; }); - } + }) }); diff --git a/ghost/admin/app/routes/member.js b/ghost/admin/app/routes/member.js index 97e474af9b..f3f14e9dfb 100644 --- a/ghost/admin/app/routes/member.js +++ b/ghost/admin/app/routes/member.js @@ -3,9 +3,10 @@ import CurrentUserSettings from 'ghost-admin/mixins/current-user-settings'; import {inject as service} from '@ember/service'; export default AuthenticatedRoute.extend(CurrentUserSettings, { - router: service(), + _requiresBackgroundRefresh: true, + init() { this._super(...arguments); this.router.on('routeWillChange', (transition) => { @@ -20,27 +21,29 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { }, model(params) { - this._isMemberUpdated = true; - return this.store.findRecord('member', params.member_id, { - reload: true - }); + this._requiresBackgroundRefresh = false; + + if (params.member_id) { + return this.store.findRecord('member', params.member_id, {reload: true}); + } else { + return this.store.createRecord('member'); + } }, - setupController(controller, model) { + setupController(controller, member) { this._super(...arguments); - if (!this._isMemberUpdated) { - controller.fetchMember.perform(model.get('id')); + if (this._requiresBackgroundRefresh) { + controller.fetchMember.perform(member.get('id')); } }, deactivate() { this._super(...arguments); - // clear the properties - let {controller} = this; - controller.model.rollbackAttributes(); - this.set('controller.model', null); - this._isMemberUpdated = false; + // clean up newly created records and revert unsaved changes to existing + this.controller.member.rollbackAttributes(); + + this._requiresBackgroundRefresh = true; }, actions: { @@ -54,10 +57,13 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { }, showUnsavedChangesModal(transition) { - if (transition.from && transition.from.name.match(/^member$/) && transition.targetName) { + if (transition.from && transition.from.name === this.routeName && transition.targetName) { let {controller} = this; - if (!controller.member.isDeleted && controller.member.hasDirtyAttributes) { + // member.changedAttributes is always true for new members but number of changed attrs is reliable + let isChanged = Object.keys(controller.member.changedAttributes()).length > 0; + + if (!controller.member.isDeleted && isChanged) { transition.abort(); controller.send('toggleUnsavedChangesModal', transition); return; diff --git a/ghost/admin/app/routes/member/new.js b/ghost/admin/app/routes/member/new.js index 87709594aa..a7bfc5325a 100644 --- a/ghost/admin/app/routes/member/new.js +++ b/ghost/admin/app/routes/member/new.js @@ -1,45 +1,6 @@ -import AuthenticatedRoute from 'ghost-admin/routes/authenticated'; -import CurrentUserSettings from 'ghost-admin/mixins/current-user-settings'; -import {isEmpty} from '@ember/utils'; -import {inject as service} from '@ember/service'; - -export default AuthenticatedRoute.extend(CurrentUserSettings, { - - router: service(), - - controllerName: 'member.new', - templateName: 'member/new', - - init() { - this._super(...arguments); - this.router.on('routeWillChange', (transition) => { - this.showUnsavedChangesModal(transition); - }); - }, - - model() { - return this.store.createRecord('member'); - }, - - // reset the model so that mobile screens react to an empty selectedMember - deactivate() { - this._super(...arguments); - - let {controller} = this; - controller.model.rollbackAttributes(); - controller.set('model', null); - }, - - showUnsavedChangesModal(transition) { - if (transition.from && transition.from.name.match(/^members\.new/) && transition.targetName) { - let {controller} = this; - let isUnchanged = isEmpty(Object.keys(controller.member.changedAttributes())); - if (!controller.member.isDeleted && !isUnchanged) { - transition.abort(); - controller.send('toggleUnsavedChangesModal', transition); - return; - } - } - } +import MemberRoute from '../member'; +export default MemberRoute.extend({ + controllerName: 'member', + templateName: 'member' }); diff --git a/ghost/admin/app/routes/tag.js b/ghost/admin/app/routes/tag.js index dba03dc7b4..aca92a8d11 100644 --- a/ghost/admin/app/routes/tag.js +++ b/ghost/admin/app/routes/tag.js @@ -6,6 +6,8 @@ import {inject as service} from '@ember/service'; export default AuthenticatedRoute.extend(CurrentUserSettings, { router: service(), + _requiresBackgroundRefresh: true, + init() { this._super(...arguments); this.router.on('routeWillChange', (transition) => { @@ -20,6 +22,8 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { }, model(params) { + this._requiresBackgroundRefresh = false; + if (params.tag_slug) { return this.store.queryRecord('tag', {slug: params.tag_slug}); } else { @@ -27,8 +31,24 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { } }, - serialize(model) { - return {tag_slug: model.get('slug')}; + serialize(tag) { + return {tag_slug: tag.get('slug')}; + }, + + setupController(controller, tag) { + this._super(...arguments); + if (this._requiresBackgroundRefresh) { + controller.fetchTag.perform(tag.get('slug')); + } + }, + + deactivate() { + this._super(...arguments); + + // clean up newly created records and revert unsaved changes to existing + this.controller.tag.rollbackAttributes(); + + this._requiresBackgroundRefresh = true; }, actions: { @@ -42,7 +62,9 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { let {controller} = this; // tag.changedAttributes is always true for new tags but number of changed attrs is reliable - if (!controller.tag.isDeleted && Object.keys(controller.tag.changedAttributes()).length > 0) { + let isChanged = Object.keys(controller.tag.changedAttributes()).length > 0; + + if (!controller.tag.isDeleted && isChanged) { transition.abort(); controller.send('toggleUnsavedChangesModal', transition); return; diff --git a/ghost/admin/app/templates/member.hbs b/ghost/admin/app/templates/member.hbs index 81cbb5da12..0dbc6abf35 100644 --- a/ghost/admin/app/templates/member.hbs +++ b/ghost/admin/app/templates/member.hbs @@ -2,58 +2,78 @@

- {{#link-to "members" data-test-link="members-back"}}Members{{/link-to}} + Members {{svg-jar "arrow-right"}} - {{#if member.name}} - {{member.name}} + {{#if this.member.isNew}} + New member {{else}} - {{member.email}} + {{or this.member.name this.member.email}} {{/if}}

- {{gh-task-button type="button" task=save class="gh-btn gh-btn-blue gh-btn-icon" data-test-button="save"}} +
+
- + {{#if (or member.name member.email)}} + + {{else}} +
+ N +
+ {{/if}}

- {{or member.name member.email}} + {{or this.member.name this.member.email}}

- {{#if (and member.name member.email)}} - {{member.email}} – + {{#if (and this.member.name this.member.email)}} + {{this.member.email}} {{/if}} - Created on {{this.subscribedAt}} + {{#unless this.member.isNew}} + {{if (and member.name member.email) "–"}} + Created on {{this.subscribedAt}} + {{/unless}}

- {{gh-member-settings-form member=member - setProperty=(action "setProperty") - isLoading=this.isLoading - showDeleteTagModal=(action "toggleDeleteMemberModal")}} + + - + {{#unless this.member.isNew}} + + {{/unless}} -{{#if showUnsavedChangesModal}} - {{gh-fullscreen-modal "leave-settings" - confirm=(action "leaveScreen") - close=(action "toggleUnsavedChangesModal") - modifier="action wide"}} +{{#if this.showUnsavedChangesModal}} + {{/if}} -{{#if showDeleteMemberModal}} - {{gh-fullscreen-modal "delete-member" - model=(hash member=member onSuccess=(action "finaliseDeletion")) - close=(action (toggle "showDeleteMemberModal" this)) - modifier="action wide"}} +{{#if this.showDeleteMemberModal}} + {{/if}} diff --git a/ghost/admin/app/templates/member/new.hbs b/ghost/admin/app/templates/member/new.hbs deleted file mode 100644 index 5a3192e6df..0000000000 --- a/ghost/admin/app/templates/member/new.hbs +++ /dev/null @@ -1,67 +0,0 @@ -
-
- -

- {{#link-to "members" data-test-link="members-back"}}Members{{/link-to}} - {{svg-jar "arrow-right"}}{{displayName}} -

-
- {{gh-task-button type="button" task=save class="gh-btn gh-btn-blue gh-btn-icon" data-test-button="save"}} -
- -
- {{#if (or member.name member.email)}} - - {{else}} -
- N -
- {{/if}} -
-

- {{#if (or member.name member.email)}} - {{or member.name member.email}} - {{else}} - New member - {{/if}} -

-

- {{#if (and member.name member.email)}} - {{member.email}} - {{/if}} - {{#unless member.isNew}} - {{if (and member.name member.email) "–"}} - Created on {{this.subscribedAt}} - {{/unless}} -

-
-
- {{gh-member-settings-form member=member - setProperty=(action "setProperty") - isLoading=this.isLoading - showDeleteTagModal=(action "toggleDeleteMemberModal")}} -
- - -
- -{{#if showUnsavedChangesModal}} - {{gh-fullscreen-modal "leave-settings" - confirm=(action "leaveScreen") - close=(action "toggleUnsavedChangesModal") - modifier="action wide"}} -{{/if}} - -{{#if showDeleteMemberModal}} - {{gh-fullscreen-modal "delete-member" - model=(hash member=member onSuccess=(action "finaliseDeletion")) - close=(action (toggle "showDeleteMemberModal" this)) - modifier="action wide"}} -{{/if}} diff --git a/ghost/admin/app/templates/members.hbs b/ghost/admin/app/templates/members.hbs index 32137bba56..375ecef838 100644 --- a/ghost/admin/app/templates/members.hbs +++ b/ghost/admin/app/templates/members.hbs @@ -3,43 +3,57 @@

Members

- {{#gh-dropdown-button dropdownName="members-actions-menu" classNames="gh-btn gh-btn-white gh-btn-icon only-has-icon gh-actions-cog" title="Members Actions" data-test-user-actions=true}} - - {{svg-jar "settings"}} - - - {{/gh-dropdown-button}} - {{#gh-dropdown name="members-actions-menu" tagName="ul" classNames="gh-member-actions-menu dropdown-menu dropdown-triangle-top-right"}} -
  • - {{#link-to "members.import" class="mr2" data-test-link="import-csv"}} - Import CSV - {{/link-to}} -
    • -
  • - - Export CSV - -
    • - {{/gh-dropdown}} + + + {{svg-jar "settings"}} + + + + +
  • + + Import CSV + +
    • +
  • + + Export CSV + +
    • +
    {{svg-jar "search" class="gh-input-search-icon"}} - +
    - {{#link-to "member.new" class="gh-btn gh-btn-green" data-test-new-member-button="true"}}New member{{/link-to}} + New member
    +
    -
      - {{#if filteredMembers}} +
        + {{#if this.filteredMembers}}
      1. {{#if this.searchText}} Search result {{else}} {{#if this.fetchMembers.lastSuccessful}} - {{pluralize memberCount "member"}} + {{pluralize this.memberCount "member"}} {{else}} Loading... {{/if}} @@ -48,19 +62,10 @@
        Created
        2. - {{#vertical-collection - items=filteredMembers - key="id" - containerSelector=".gh-main" - estimateHeight=60 - bufferSize=20 - as |member| - }} - {{gh-members-list-item - member=member - data-test-member-id=member.id - }} - {{/vertical-collection}} + + + + {{else}} {{#if this.fetchMembers.isRunning}}
        @@ -84,4 +89,5 @@
    + {{outlet}} \ No newline at end of file diff --git a/ghost/admin/app/templates/tag.hbs b/ghost/admin/app/templates/tag.hbs index 8e21a17b95..a5fdfb76de 100644 --- a/ghost/admin/app/templates/tag.hbs +++ b/ghost/admin/app/templates/tag.hbs @@ -2,37 +2,41 @@

    - {{#link-to "tags" data-test-link="tags-back"}}Tags{{/link-to}} + Tags {{svg-jar "arrow-right"}} - {{if tag.isNew "New tag" tag.name}} + {{if this.tag.isNew "New tag" tag.name}}

    - {{gh-task-button task=save type="button" class="gh-btn gh-btn-blue gh-btn-icon" data-test-button="save"}} +
    - {{gh-tag-settings-form tag=tag - setProperty=(action "setProperty") - showDeleteTagModal=(action "toggleDeleteTagModal")}} + + {{#unless this.tag.isNew}} - {{/unless}} {{#if showUnsavedChangesModal}} - {{gh-fullscreen-modal "leave-settings" - confirm=(action "leaveScreen") - close=(action "toggleUnsavedChangesModal") - modifier="action wide"}} + {{/if}} {{#if showDeleteTagModal}} -{{gh-fullscreen-modal "delete-tag" - model=tag - confirm=(action "deleteTag") - close=(action "toggleDeleteTagModal") - modifier="action wide"}} + {{/if}} \ No newline at end of file diff --git a/ghost/admin/app/templates/tags.hbs b/ghost/admin/app/templates/tags.hbs index 90a3475e0a..728afd4302 100644 --- a/ghost/admin/app/templates/tags.hbs +++ b/ghost/admin/app/templates/tags.hbs @@ -3,42 +3,33 @@

    Tags

    - - + +
    - {{#link-to "tag.new" class="gh-btn gh-btn-green"}}New tag{{/link-to}} + New tag
    +
    -
      - {{#if sortedTags}} +
        + {{#if this.sortedTags}}
      1. Tag
        Slug
        No. of posts
        2. - {{#vertical-collection - items=sortedTags - key="id" - containerSelector=".gh-main" - estimateHeight=60 - bufferSize=20 - as |tag| - }} - {{gh-tags-list-item - tag=tag - data-test-tag-id=tag.id - }} - {{/vertical-collection}} + + + {{else}}
      2. {{svg-jar "tags-placeholder" class="gh-tags-placeholder"}} -

        You haven't created any {{type}} tags yet!

        - {{#link-to "tag.new" class="gh-btn gh-btn-green gh-btn-lg"}} +

        You haven't created any {{this.type}} tags yet!

        + Create a new tag - {{/link-to}} +
        3. {{/if}}