chore: convert `Dropdown` components to TS #3608

SychO9 · 2022-08-21T17:54:15Z

Changes proposed in this pull request:
Converts the Dropdown component and its descendants to TS.

Necessity

Has the problem that is being solved here been clearly explained?
If applicable, have various options for solving this problem been considered?
For core PRs, does this need to be in core, or could it be in an extension?
Are we willing to maintain this for years / potentially forever?

Confirmed

Frontend changes: tested on a local Flarum installation.
Backend changes: tests are green (run composer test).
Core developer confirmed locally this works as intended.
Tests have been added, or are not appropriate here.

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

framework/core/js/src/common/components/Dropdown.tsx

framework/core/js/src/common/components/SelectDropdown.tsx

framework/core/js/src/common/components/SplitDropdown.tsx

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

Co-authored-by: David Wheatley <hi@davwheat.dev>

framework/core/js/src/admin/components/PermissionDropdown.tsx

askvortsov1 · 2022-09-06T11:18:58Z

framework/core/js/src/admin/components/PermissionDropdown.tsx

-                disabled: this.isGroupDisabled(group.id()) && this.isGroupDisabled(Group.MEMBER_ID) && this.isGroupDisabled(Group.GUEST_ID),
+      const groupButtons = app.store
+        .all<Group>('groups')
+        .filter((group) => ![Group.ADMINISTRATOR_ID, Group.GUEST_ID, Group.MEMBER_ID].includes(group.id()!))


Can we break this array out into a constant? Also, is it just me, or did we have a groups selector component at one point?

This component itself is the selector you're referring to I believe. It doesn't just list groups though, but different times of items depending on the permission.

framework/core/js/src/admin/components/PermissionDropdown.tsx

framework/core/js/src/common/components/Dropdown.tsx

framework/core/js/src/common/components/SelectDropdown.tsx

askvortsov1 · 2022-09-06T11:25:27Z

framework/core/js/src/common/components/SelectDropdown.tsx

+
+  getButtonContent(children: Mithril.ChildArray): Mithril.ChildArray {
+    const activeChild = children.find(isActive);
+    let label = (activeChild && typeof activeChild === 'object' && 'children' in activeChild && activeChild.children) || this.attrs.defaultLabel;


We should really consider investing in a util file full of these guards/

askvortsov1 · 2022-09-06T11:26:02Z

framework/core/js/src/common/components/SplitDropdown.tsx

+    // these attrs to a new button, so that it has exactly the same behaviour as
+    // the first child.
+    const firstChild = this.getFirstChild(children);
+    const buttonAttrs = Object.assign({}, firstChild?.attrs);


Suggested change

const buttonAttrs = Object.assign({}, firstChild?.attrs);

const buttonAttrs = Object.assign({}, firstChild?.attrs ?? {});

Is this necessary?

Why not

{ ...firstChild?.attrs ?? {} }

I don't think this is necessary, the getFirstChild method is typed to explicitly return a Vnode. if somehow it returns a non Vnode then it should probably be fixed at that level?

framework/core/js/src/common/components/SplitDropdown.tsx

askvortsov1 · 2022-09-06T11:27:22Z

framework/core/js/src/forum/ForumApplication.ts

@@ -126,7 +126,7 @@ export default class ForumApplication extends Application {
    // Route the home link back home when clicked. We do not want it to register
    // if the user is opening it in a new tab, however.
    document.getElementById('home-link')!.addEventListener('click', (e) => {
-      if (e.ctrlKey || e.metaKey || e.which === 2) return;
+      if (e.ctrlKey || e.metaKey || e.button === 1) return;


Let's add a comment explaining this one

are you referring to the conditional statement or just the e.button part?

I think this is quite self-explanatory?

The comment above explains everything this line does.

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

davwheat

Approved, tentative on #3608 (comment)

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

iPurpl3x · 2023-03-16T13:58:55Z

These changes seem to have broken implementations like LanguageDropdown. Seems like the label text can't be extracted from complex markup like the one used in LanguageDropdown...

cc @imorland, @SychO9

chore: convert Dropdown components to TS

e8a8bd4

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

SychO9 added the type/cleanup label Aug 21, 2022

SychO9 requested a review from davwheat August 21, 2022 17:54

davwheat reviewed Aug 21, 2022

View reviewed changes

SychO9 self-assigned this Aug 23, 2022

SychO9 and others added 5 commits August 31, 2022 18:35

chore(review): buttonClassName technically not required

6207c12

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

chore(review): accessibleToggleLabel technically not required

c0b3cdb

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

chore(review): use classList where possible

27d26de

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

chore: yarn format

530d7b5

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

Update framework/core/js/src/common/components/Dropdown.tsx

29f7500

Co-authored-by: David Wheatley <hi@davwheat.dev>

SychO9 requested a review from davwheat August 31, 2022 17:40

askvortsov1 requested changes Sep 6, 2022

View reviewed changes

SychO9 added this to the 1.6 milestone Sep 6, 2022

SychO9 added 6 commits September 7, 2022 11:28

chore(review): use includes

ac413fa

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

chore(review): define constant of excluded groups

dab4131

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

chore(review): use null coalesce and logical or assignments

daab50c

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

chore(review): null coalesce

c9e997c

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

chore(review): any to typeof Component

14259de

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

chore(review): classList

390edaf

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

SychO9 requested a review from a team as a code owner September 7, 2022 10:59

chore(review): yarn format

03c853d

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

davwheat approved these changes Oct 11, 2022

View reviewed changes

SychO9 modified the milestones: 1.6, 1.7 Nov 15, 2022

SychO9 added 2 commits November 26, 2022 14:12

Merge branch 'main' into sm/dropdown-typescript

e0c7afb

chore: fix typing issues after typescript update

38ba49e

Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>

SychO9 requested a review from imorland November 26, 2022 15:13

imorland approved these changes Nov 27, 2022

View reviewed changes

SychO9 merged commit 5bc47c0 into main Nov 27, 2022

SychO9 deleted the sm/dropdown-typescript branch November 27, 2022 09:34

imorland mentioned this pull request Nov 27, 2022

fix: undefined showing in dropdown active title #3700

Merged

10 tasks

SychO9 added the javascript Pull requests that update Javascript code label Feb 11, 2023

imorland mentioned this pull request Mar 21, 2023

fix: empty string displayed as SelectDropdown title #3773

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: convert `Dropdown` components to TS #3608

chore: convert `Dropdown` components to TS #3608

SychO9 commented Aug 21, 2022 •

edited

Loading

askvortsov1 Sep 6, 2022

SychO9 Sep 7, 2022

askvortsov1 Sep 6, 2022

askvortsov1 Sep 6, 2022

davwheat Sep 6, 2022

SychO9 Sep 7, 2022

askvortsov1 Sep 6, 2022

SychO9 Sep 7, 2022

davwheat Oct 11, 2022

davwheat left a comment

iPurpl3x commented Mar 16, 2023

	const buttonAttrs = Object.assign({}, firstChild?.attrs);
	const buttonAttrs = Object.assign({}, firstChild?.attrs ?? {});

chore: convert Dropdown components to TS #3608

chore: convert Dropdown components to TS #3608

Conversation

SychO9 commented Aug 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davwheat left a comment

Choose a reason for hiding this comment

iPurpl3x commented Mar 16, 2023

chore: convert `Dropdown` components to TS #3608

chore: convert `Dropdown` components to TS #3608

SychO9 commented Aug 21, 2022 •

edited

Loading