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

Frontend Refactor #59

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Frontend Refactor #59

wants to merge 12 commits into from

Conversation

DavideIadeluca
Copy link
Member

@DavideIadeluca DavideIadeluca commented Nov 2, 2024

Fixes #0000

Changes proposed in this pull request:

  • Target 1.x workflows
  • Use frontend extenders
  • TS Refactor

Reviewers should focus on:

  • I tried to properly use TS as far as reasonably possible. At some point I had to stop because logic would have to be rewritten and I'm afraid that I might introduce regression.

Screenshot

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes:

@DavideIadeluca DavideIadeluca added the enhancement New feature or request label Nov 2, 2024
@DavideIadeluca DavideIadeluca self-assigned this Nov 2, 2024
@DavideIadeluca DavideIadeluca marked this pull request as ready for review November 2, 2024 09:48
@DavideIadeluca
Copy link
Member Author

@dsevillamartin I would be glad if you could also take a glance at this, particularly the changes in the UserBio component itself

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.

Overall looks good, haven't tested locally though.

import type User from 'flarum/common/models/User';

export default function addBioToUserCard() {
extend(UserCard.prototype, 'infoItems', function (items: ItemList<Mithril.Children>) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd type this in the function itself - I believe this should work. (Does UserCard not have the attributes typed?)

Suggested change
extend(UserCard.prototype, 'infoItems', function (items: ItemList<Mithril.Children>) {
extend(UserCard.prototype, 'infoItems', function (items: ItemList<Mithril.Children>, this: any) {

*/
this.textareaRows = '5';
export default class UserBio extends Component<UserBioAttrs> {
editing: boolean = false;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if having values moved from oninit to instance creation causes any issues or not... theoretically oninit should only be called once, and since these are primitives it shouldn't really matter as there are no side effects? Just some thoughts.

const index = lengthBefore + lineIndex;

// Show the same number of lines to avoid layout shift
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Why are all these ts-ignores necessary?

@imorland
Copy link
Member

imorland commented Nov 3, 2024

Please note a minor change I made today in the released version:

'<p>' + $('<div/>').text(user.bio()).html().replace(/\n/g, '<br>').autoLink({ rel: 'nofollow ugc', target: '_blank' }) + '</p>'

(added ugc and target="_blank" by default)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants