Skip to content

Export more for header #2996

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.

Sign up for GitHub

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

Jump to bottom
Merged
blessedcoolant merged 9 commits into from
Mar 22, 2023
Merged

Export more for header #2996

blessedcoolant merged 9 commits into from
Mar 22, 2023

Conversation

@maryhipp
Copy link
Contributor

@maryhipp maryhipp commented Mar 20, 2023 •
edited
Loading

  • export more items needed for dynamic header
  • remove build mode that is no longer needed

psychedelicious
psychedelicious reviewed Mar 20, 2023
View reviewed changes
Copy link
Contributor

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Why are the components exported as classes extending React.Component? Wouldn't it be React.FunctionComponent given these are function components?

psychedelicious
psychedelicious reviewed Mar 20, 2023
View reviewed changes
Copy link
Contributor

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Why are the components exported as classes extending React.Component? Wouldn't it be React.FunctionComponent given these are function components?

@maryhipp
Copy link
Contributor Author

maryhipp commented Mar 21, 2023

Probably because whatever stackoverflow I copied that from used React.Component and it worked 😂 I can update to FunctionComponent

@maryhipp
Copy link
Contributor Author

maryhipp commented Mar 21, 2023

I tried changing this to both React.FunctionComponent and JSX.Element and they both presented a slew of type errors in my React app that is rendering this - are you ok to leave as React.Component for now since that seems to play nicely for whatever reason?

@psychedelicious
Copy link
Contributor

psychedelicious commented Mar 21, 2023

Haha, yep gotcha. Maybe ReactNode is the correct typing for a component - I know that's what you'd use if you want a component to have children, but I'm not sure if that is correct here either. If this works, no worries.

I've approved, feel free to rebase and merge at your leisure, or change it if you want.

@maryhipp
Copy link
Contributor Author

maryhipp commented Mar 22, 2023

@blessedcoolant will you take a look when you have a chance please?

@blessedcoolant blessedcoolant merged commit 485f6e5 into invoke-ai:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blessedcoolant blessedcoolant Awaiting requested review from blessedcoolant blessedcoolant is a code owner

1 more reviewer

@psychedelicious psychedelicious psychedelicious approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@maryhipp @psychedelicious @blessedcoolant