From 99deb2514db9402338617822b9ba125dc4442717 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 14 May 2021 21:04:26 -0400 Subject: [PATCH 1/5] NotificationListState separate content method This fixes an error where an empty notification list wouldn't show the "empty" text. It also simplifies flow of logic and breaks the component up a bit for readability. --- .../src/forum/components/NotificationList.js | 126 +++++++++--------- 1 file changed, 63 insertions(+), 63 deletions(-) diff --git a/framework/core/js/src/forum/components/NotificationList.js b/framework/core/js/src/forum/components/NotificationList.js index 22a337efa..f469dd00d 100644 --- a/framework/core/js/src/forum/components/NotificationList.js +++ b/framework/core/js/src/forum/components/NotificationList.js @@ -12,7 +12,6 @@ import Discussion from '../../common/models/Discussion'; export default class NotificationList extends Component { view() { const state = this.attrs.state; - const pages = state.getPages(); return (
@@ -29,72 +28,73 @@ export default class NotificationList extends Component {
-
- {state.hasItems() - ? pages.map((page) => { - const groups = []; - const discussions = {}; - - page.items.forEach((notification) => { - const subject = notification.subject(); - - if (typeof subject === 'undefined') return; - - // Get the discussion that this notification is related to. If it's not - // directly related to a discussion, it may be related to a post or - // other entity which is related to a discussion. - let discussion = null; - if (subject instanceof Discussion) discussion = subject; - else if (subject && subject.discussion) discussion = subject.discussion(); - - // If the notification is not related to a discussion directly or - // indirectly, then we will assign it to a neutral group. - const key = discussion ? discussion.id() : 0; - discussions[key] = discussions[key] || { discussion: discussion, notifications: [] }; - discussions[key].notifications.push(notification); - - if (groups.indexOf(discussions[key]) === -1) { - groups.push(discussions[key]); - } - }); - - return groups.map((group) => { - const badges = group.discussion && group.discussion.badges().toArray(); - - return ( -
- {group.discussion ? ( - - {badges && badges.length &&
    {listItems(badges)}
} - {group.discussion.title()} - - ) : ( -
{app.forum.attribute('title')}
- )} - -
    - {group.notifications.map((notification) => { - const NotificationComponent = app.notificationComponents[notification.contentType()]; - return NotificationComponent ?
  • {NotificationComponent.component({ notification })}
    • : ''; - })} -
-
- ); - }); - }) - : ''} - {state.isLoading() ? ( - - ) : pages.length ? ( - '' - ) : ( -
{app.translator.trans('core.forum.notifications.empty_text')}
- )} -
+
{this.content(state)}
); } + content(state) { + if (state.isLoading()) { + return ; + } + + if (state.hasItems()) { + return state.getPages().map((page) => { + const groups = []; + const discussions = {}; + + page.items.forEach((notification) => { + const subject = notification.subject(); + + if (typeof subject === 'undefined') return; + + // Get the discussion that this notification is related to. If it's not + // directly related to a discussion, it may be related to a post or + // other entity which is related to a discussion. + let discussion = null; + if (subject instanceof Discussion) discussion = subject; + else if (subject && subject.discussion) discussion = subject.discussion(); + + // If the notification is not related to a discussion directly or + // indirectly, then we will assign it to a neutral group. + const key = discussion ? discussion.id() : 0; + discussions[key] = discussions[key] || { discussion: discussion, notifications: [] }; + discussions[key].notifications.push(notification); + + if (groups.indexOf(discussions[key]) === -1) { + groups.push(discussions[key]); + } + }); + + return groups.map((group) => { + const badges = group.discussion && group.discussion.badges().toArray(); + + return ( +
+ {group.discussion ? ( + + {badges && badges.length &&
    {listItems(badges)}
} + {group.discussion.title()} + + ) : ( +
{app.forum.attribute('title')}
+ )} + +
    + {group.notifications.map((notification) => { + const NotificationComponent = app.notificationComponents[notification.contentType()]; + return NotificationComponent ?
  • {NotificationComponent.component({ notification })}
    • : ''; + })} +
+
+ ); + }); + }); + } + + return
{app.translator.trans('core.forum.notifications.empty_text')}
; + } + oncreate(vnode) { super.oncreate(vnode); From 0289a3b71468dc27797d9862ce685fff1fb6fa71 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 14 May 2021 21:08:48 -0400 Subject: [PATCH 2/5] Fix 0s in notification dropdown By casting the length int to a bool, if there are no badges, we don't display a 0. It seems that mithril will render integers, but not booleans. Fixes https://github.com/flarum/QualityAssurance/issues/28 --- framework/core/js/src/forum/components/NotificationList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/js/src/forum/components/NotificationList.js b/framework/core/js/src/forum/components/NotificationList.js index f469dd00d..be100e780 100644 --- a/framework/core/js/src/forum/components/NotificationList.js +++ b/framework/core/js/src/forum/components/NotificationList.js @@ -73,7 +73,7 @@ export default class NotificationList extends Component {
{group.discussion ? ( - {badges && badges.length &&
    {listItems(badges)}
} + {badges && !!badges.length &&
    {listItems(badges)}
} {group.discussion.title()} ) : ( From c90f05cb942de9475ca6415f8b183636faa00866 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 14 May 2021 21:21:58 -0400 Subject: [PATCH 3/5] Fix KeyboardNavigatable In acd3873bbd46e92510ebf92e7182324e1c6d540e, I tried to be clever and create a new KeyboardNavigatable object as a return value for `when`. My approach to cloning was incorrect, and caused the util to break entirely. My original intent for having this "clone"-based behavior is that a single KeyboardNavigatable instance could be created with multiple listeners, and then "cloned" like this with different "activators" registered via "then" calls. In hindsight, this change introduces more issues than it solves: outside of just not working, the cloned "KeyboardNavigatable" instances have shared internal state (the set of callbacks), and each has write access to this internal state. This is a recipe for unpredictable behavior and confusing bugs, so best to keep things simple for now, and maybe introduce more functional behavior in later releases. Fixes https://github.com/flarum/QualityAssurance/issues/25 --- framework/core/js/src/forum/utils/KeyboardNavigatable.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/framework/core/js/src/forum/utils/KeyboardNavigatable.ts b/framework/core/js/src/forum/utils/KeyboardNavigatable.ts index 1e9dc5568..739a590fc 100644 --- a/framework/core/js/src/forum/utils/KeyboardNavigatable.ts +++ b/framework/core/js/src/forum/utils/KeyboardNavigatable.ts @@ -104,7 +104,9 @@ export default class KeyboardNavigatable { * Provide a callback that determines whether keyboard input should be handled. */ when(callback: ShouldHandle): KeyboardNavigatable { - return { ...this, whenCallback: callback }; + this.whenCallback = callback; + + return this; } /** From 4436d82c36852b9d2693007e6ef52e55b496dcd5 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 14 May 2021 21:36:06 -0400 Subject: [PATCH 4/5] Fix CustomFooterModal Appearance The textarea in the CustomFooterModal was much larger than in the other appearance page modals, and did not use a monospaced font. Turns out the other 2 were explicitly specified in the less. This commit adds a class that can be applied to all these modals for simpler maintenance. Fixes https://github.com/flarum/core/issues/2865 --- framework/core/js/src/admin/components/EditCustomCssModal.js | 2 +- framework/core/js/src/admin/components/EditCustomFooterModal.js | 2 +- framework/core/js/src/admin/components/EditCustomHeaderModal.js | 2 +- framework/core/less/admin/AppearancePage.less | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/framework/core/js/src/admin/components/EditCustomCssModal.js b/framework/core/js/src/admin/components/EditCustomCssModal.js index c7686b085..bd855d545 100644 --- a/framework/core/js/src/admin/components/EditCustomCssModal.js +++ b/framework/core/js/src/admin/components/EditCustomCssModal.js @@ -2,7 +2,7 @@ import SettingsModal from './SettingsModal'; export default class EditCustomCssModal extends SettingsModal { className() { - return 'EditCustomCssModal Modal--large'; + return 'EditCustomCssModal TextareaCodeModal Modal--large'; } title() { diff --git a/framework/core/js/src/admin/components/EditCustomFooterModal.js b/framework/core/js/src/admin/components/EditCustomFooterModal.js index 88d97535e..43cfd29d4 100644 --- a/framework/core/js/src/admin/components/EditCustomFooterModal.js +++ b/framework/core/js/src/admin/components/EditCustomFooterModal.js @@ -2,7 +2,7 @@ import SettingsModal from './SettingsModal'; export default class EditCustomFooterModal extends SettingsModal { className() { - return 'EditCustomFooterModal Modal--large'; + return 'EditCustomFooterModal TextareaCodeModal Modal--large'; } title() { diff --git a/framework/core/js/src/admin/components/EditCustomHeaderModal.js b/framework/core/js/src/admin/components/EditCustomHeaderModal.js index 24625cbd7..11a1f8d4e 100644 --- a/framework/core/js/src/admin/components/EditCustomHeaderModal.js +++ b/framework/core/js/src/admin/components/EditCustomHeaderModal.js @@ -2,7 +2,7 @@ import SettingsModal from './SettingsModal'; export default class EditCustomHeaderModal extends SettingsModal { className() { - return 'EditCustomHeaderModal Modal--large'; + return 'EditCustomHeaderModal TextareaCodeModal Modal--large'; } title() { diff --git a/framework/core/less/admin/AppearancePage.less b/framework/core/less/admin/AppearancePage.less index 18b5a46e7..0e7ba3471 100644 --- a/framework/core/less/admin/AppearancePage.less +++ b/framework/core/less/admin/AppearancePage.less @@ -37,7 +37,7 @@ margin-bottom: 15px; } -.EditCustomCssModal, .EditCustomHeaderModal { +.TextareaCodeModal { textarea { font-family: monospace; line-height: 1; From e835e36b8e8e85efad2f4be936d50c6373960ef7 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Sat, 15 May 2021 02:30:03 -0400 Subject: [PATCH 5/5] Fix glitchy group editing Currently, when groups are edited, the new groups flicker, but the UI soon reverts to the old groups. This is because the returned API response has the old group values. This, in turn, is because we eager load groups, and when we sync the new group relation, that doesn't update the groups saved in memory. By unsetting the relation, we make sure the right groups are returned (and also available to the GroupsChanged event). See https://github.com/flarum/core/issues/2514 --- framework/core/src/User/Command/EditUserHandler.php | 1 + 1 file changed, 1 insertion(+) diff --git a/framework/core/src/User/Command/EditUserHandler.php b/framework/core/src/User/Command/EditUserHandler.php index cdf183e51..51cdd6264 100644 --- a/framework/core/src/User/Command/EditUserHandler.php +++ b/framework/core/src/User/Command/EditUserHandler.php @@ -130,6 +130,7 @@ class EditUserHandler $user->afterSave(function (User $user) use ($newGroupIds) { $user->groups()->sync($newGroupIds); + $user->unsetRelation('groups'); }); }