From 8ffeac43159c39392117e0bd767de62e49758ebd 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. --- js/src/forum/components/NotificationList.js | 126 ++++++++++---------- 1 file changed, 63 insertions(+), 63 deletions(-) diff --git a/js/src/forum/components/NotificationList.js b/js/src/forum/components/NotificationList.js index 22a337efa..f469dd00d 100644 --- a/js/src/forum/components/NotificationList.js +++ b/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 d1e987a2402eec3a6225533dc72d321185537680 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 --- js/src/forum/components/NotificationList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/forum/components/NotificationList.js b/js/src/forum/components/NotificationList.js index f469dd00d..be100e780 100644 --- a/js/src/forum/components/NotificationList.js +++ b/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 05dda5b083e0a52894ef30dcb2cb13331cd74025 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 b2d053f6865e685ebf005e457d970385377bbb28, 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 --- js/src/forum/utils/KeyboardNavigatable.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/js/src/forum/utils/KeyboardNavigatable.ts b/js/src/forum/utils/KeyboardNavigatable.ts index 1e9dc5568..739a590fc 100644 --- a/js/src/forum/utils/KeyboardNavigatable.ts +++ b/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 9eb74fdc8a37facdb98ed77c9fca92542962d054 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 --- js/src/admin/components/EditCustomCssModal.js | 2 +- js/src/admin/components/EditCustomFooterModal.js | 2 +- js/src/admin/components/EditCustomHeaderModal.js | 2 +- less/admin/AppearancePage.less | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/js/src/admin/components/EditCustomCssModal.js b/js/src/admin/components/EditCustomCssModal.js index c7686b085..bd855d545 100644 --- a/js/src/admin/components/EditCustomCssModal.js +++ b/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/js/src/admin/components/EditCustomFooterModal.js b/js/src/admin/components/EditCustomFooterModal.js index 88d97535e..43cfd29d4 100644 --- a/js/src/admin/components/EditCustomFooterModal.js +++ b/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/js/src/admin/components/EditCustomHeaderModal.js b/js/src/admin/components/EditCustomHeaderModal.js index 24625cbd7..11a1f8d4e 100644 --- a/js/src/admin/components/EditCustomHeaderModal.js +++ b/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/less/admin/AppearancePage.less b/less/admin/AppearancePage.less index 18b5a46e7..0e7ba3471 100644 --- a/less/admin/AppearancePage.less +++ b/less/admin/AppearancePage.less @@ -37,7 +37,7 @@ margin-bottom: 15px; } -.EditCustomCssModal, .EditCustomHeaderModal { +.TextareaCodeModal { textarea { font-family: monospace; line-height: 1; From fede3f9fc7e0a6cb35353db3f39cb7c70ead8d3a 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 --- src/User/Command/EditUserHandler.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/User/Command/EditUserHandler.php b/src/User/Command/EditUserHandler.php index cdf183e51..51cdd6264 100644 --- a/src/User/Command/EditUserHandler.php +++ b/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'); }); }