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

Convert modding profile component to typescript #10311

Merged
merged 19 commits into from
Jul 6, 2023

Conversation

notbakaneko
Copy link
Collaborator

@notbakaneko notbakaneko commented Jun 26, 2023

now using a copy of the pageScan/Jump logic from the main profile page (also there's an existing problem of lazy loaded images shifting the scroll position)


import UserExtendedJson from './user-extended-json';

type ProfileHeaderIncludes =
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should go in user-json and also used for typing in profile-page/extra-page-props.

Comment on lines 168 to 171
// add space for warning banner when user is blocked
modifiers={{
restricted: core.currentUserModel.blocks.has(this.props.user.id) || this.props.user.is_restricted,
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

apparently I missed this one but this doesn't do anything anymore

</div>
<div
ref={this.pagesOffsetRef}
className='hidden-xs page-extra-tabs page-extra-tabs--profile-page'
Copy link
Collaborator

Choose a reason for hiding this comment

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

the hidden-xs shouldn't be here anymore

}

let preferred: Page | undefined;
const pageIds = [...matching.values()];
Copy link
Collaborator

Choose a reason for hiding this comment

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

.values() not needed?

@@ -4,4 +4,4 @@
import ReviewEditorConfigJson from 'interfaces/review-editor-config-json';
import * as React from 'react';

export const ReviewEditorConfigContext = React.createContext({} as ReviewEditorConfigJson);
export const ReviewEditorConfigContext = React.createContext<ReviewEditorConfigJson>({ max_blocks: 0 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's this change doing here 🤔 although I guess it's simple enough so this can stay

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

max_blocks isn't supposed to be undefined and this just makes the default 0 so pages that don't need editing can skip adding a provider without causing max_blocks to be undefined

);
}

private extraPage = (name: ModdingExtraPage) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think some of the components should be observer? ...or not? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder about that 🤔 (apparently everything that receives an observable prop should also be an observer, otherwise it's supposed to be toJSd even if the props don't change? 🤔 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah because otherwise when passing object and any of its properties get updated the non-observer component will be outdated. The toJS tells that the current component do access the properties so it'll update its children accordingly. Although in this case there's no update involved so it should be fine...? I'd probably still make them observer anyway just in case.

getSnapshotBeforeUpdate() needs to return a snapshot or null; we don't really need to handle it on the modding profile
page at the moment since the only lazy loaded section is the last one.
@nanaya nanaya enabled auto-merge July 6, 2023 09:53
@nanaya nanaya merged commit 70f8e36 into ppy:master Jul 6, 2023
3 checks passed
@notbakaneko notbakaneko deleted the feature/ts-modding-profile-main branch July 31, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants