Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.2 JS clean-up #3214

Merged
merged 11 commits into from
Dec 27, 2021
Merged

1.2 JS clean-up #3214

merged 11 commits into from
Dec 27, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented Dec 20, 2021

Changes proposed in this pull request:

Performs some clean-up to frontend JS, ready for 1.2.

  • fixes extend.ts typing issue introduced in Convert extend util to TypeScript #2928
  • fixes bad JS docblocks (contained within f476c2b)
  • remove fallbacks for window.requestAnimationFrame() -- this is supported in all browsers now
  • consistently return null instead of '' or [] for empty vdoms
  • simplify some code and checks inside Post, PostsUserPage and slideable (40be94d)

Updating our JSDoc blocks allows us to have some form of type safety and hinting without needing to fully convert to Typescript just yet.

Reviewers should focus on:

N/A

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.

@davwheat davwheat added this to the 1.2 milestone Dec 20, 2021
@davwheat davwheat self-assigned this Dec 20, 2021
@davwheat davwheat mentioned this pull request Dec 20, 2021
8 tasks
js/src/common/extend.ts Outdated Show resolved Hide resolved
js/src/admin/components/BasicsPage.js Outdated Show resolved Hide resolved
js/src/forum/components/NotificationGrid.js Outdated Show resolved Hide resolved
js/src/forum/components/Post.js Outdated Show resolved Hide resolved
js/src/forum/utils/slidable.js Outdated Show resolved Hide resolved
Comment on lines 22 to 24
content() {
return [];
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should really all become abstract methods for v2

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as long as the last few NestedStringArray are changed to Mithril.Children

js/src/forum/components/EventPost.js Outdated Show resolved Hide resolved
js/src/forum/components/NotificationGrid.js Outdated Show resolved Hide resolved
js/src/forum/components/NotificationGrid.js Outdated Show resolved Hide resolved
js/src/forum/components/NotificationGrid.js Outdated Show resolved Hide resolved
Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not tested locally (shouldn't need to) & changes are mostly docblocks which don't break anything. Nothing stands out to me so LGTM.

@davwheat davwheat merged commit d268894 into master Dec 27, 2021
@davwheat davwheat deleted the dw/t1.2-cleanup branch December 27, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants