Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix: pass config prop to the renderer #96

Closed
wants to merge 3 commits into from

Conversation

KhubaibQaiser
Copy link

@KhubaibQaiser KhubaibQaiser commented Aug 8, 2024

closes #49

Copy link
Owner

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Please make changes without formatting diffs

@KhubaibQaiser
Copy link
Author

Please make changes without formatting diffs

Yeah, sorry about that. I have fixed the formatting issue.

@KhubaibQaiser
Copy link
Author

Hi @ShaMan123 , could you please review the changes, merge the PR and push a release. This would help so many waiting for the fix.

Copy link
Owner

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

I will add a github publish action

package.json Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

please undo this change, npm scripts handle semver changes

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines +45 to +46
const { config: configProps } = props as MathTextItemProps<true>;
const config = useMemo(() => ({ ...(configProps || {}), inline }), [inline, configProps]);
Copy link
Owner

Choose a reason for hiding this comment

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

If config is always passed as optional then you should change MathTextItemProps it seems.
config?: MathViewProps['config']

https://github.com/ShaMan123/react-native-math-view/blob/a089f98a28d278a6a52e8ed6891e114fff50e588/src/common.tsx#L22C22-L22C37

Copy link
Owner

Choose a reason for hiding this comment

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

Is that correct?

Copy link
Author

@KhubaibQaiser KhubaibQaiser Aug 12, 2024

Choose a reason for hiding this comment

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

The MathTextItemProps extends from MathViewCommonProps which defines config as optional, which I guess is intentional? Because if you look at the MathTextItemProps it optionally extends from MathViewCommonProps if the generic type is a boolean. Not sure what the purpose is for this optional extension.

export type MathTextItemProps<T extends boolean = boolean> = (T extends true ? Omit<MathViewProps, 'math'> : TextProps) & {
    value: string,
    isMath: T,
    Component?: MathView,
    CellRendererComponent?: ElementOrRenderer,
    inline?: boolean
}

Copy link
Owner

Choose a reason for hiding this comment

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

So I prefer to spread config: configProp in line 43 if that is possible.
Sorry for the delay and for not being acquainted with the code anymore.

Actually, I think it is best you fork and I archive this repo with a pinned issue stating so so other devs can write there what they want.

@ShaMan123
Copy link
Owner

#97

I suggest you fork

@ShaMan123 ShaMan123 closed this Sep 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

font size config not working for MathText
2 participants