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.

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

AppBase - additional refactor to improve minimal app size #4175

Merged
merged 8 commits into from
Apr 6, 2022
Merged

AppBase - additional refactor to improve minimal app size #4175

merged 8 commits into from
Apr 6, 2022

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Apr 5, 2022 •
edited
Loading

  • deprecated Application.enableVr and Application.disableVr
  • AppBase no longer imports: VrMnager, XrManager and BatchManager (those are optional now)

@mvaligursky mvaligursky marked this pull request as draft April 5, 2022 17:09
@mvaligursky mvaligursky changed the title AppBase - additional refactor AppBase - additional refactor ti improve minimal app size Apr 6, 2022
@mvaligursky mvaligursky self-assigned this Apr 6, 2022
@mvaligursky mvaligursky requested a review from a team April 6, 2022 09:28
@mvaligursky mvaligursky marked this pull request as ready for review April 6, 2022 09:28
@yaustar yaustar changed the title AppBase - additional refactor ti improve minimal app size AppBase - additional refactor to improve minimal app size Apr 6, 2022
@willeastcott
Copy link
Contributor

The naming for AppCreateOptions suggests to me it's used on creation (i.e. in the constructor). So what about AppInitOptions or just AppOptions?

willeastcott
willeastcott approved these changes Apr 6, 2022
View reviewed changes
Copy link
Contributor

@willeastcott willeastcott left a comment

Choose a reason for hiding this comment

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

I've left a couple of comments laying out some concerns I have. But approving for now since I don't want to delay anything. BTW, I think this is a really nice step forwards overall!!! 🚀

@mvaligursky mvaligursky merged commit 97d6f44 into main Apr 6, 2022
@mvaligursky mvaligursky deleted the mvaligursky-appbase2 branch April 6, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willeastcott willeastcott willeastcott approved these changes

Assignees

@mvaligursky mvaligursky

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvaligursky @willeastcott