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

Chore: Cleanup document for renderer options #9120

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

SuperSodaSea
Copy link
Member

@SuperSodaSea SuperSodaSea commented Jan 30, 2023

Description of change

Trying to cleanup IRendererOptions related documents. Previously these documents were not very consistent in different places, now they are much better. Also I rewritten some of the descriptions.

Document preview:

Also settings.RENDER_OPTIONS.legacy is removed since it is no longer being used.

Fixes #8073. Cleanup for v6.x branch is in #9123.

Pre-Merge Checklist

  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@SuperSodaSea SuperSodaSea added this to the v7.1.2 milestone Jan 30, 2023
@SuperSodaSea
Copy link
Member Author

BTW it seems that settings.RENDER_OPTIONS.legacy is not used anywhere in the code (Filter do have a legacy, but it have nothing to do with the previous one). Should we remove it?

@bigtimebuddy
Copy link
Member

Yes, please remove legacy. That was used a long time ago with Android browser compatibility. Most of that is now irrelevant.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

This is great! One minor suggestion, could you alphabetize the options to make it more organized (e.g., Renderer's options)

@SuperSodaSea
Copy link
Member Author

Oh, I meant to order them by their function (options for ViewSystem, BackgroundSystem, ContextSystem, etc.). If you insist on alphabetical order, I can change them.

@bigtimebuddy
Copy link
Member

Yeah, for most people, the systems are an implementation detail they don't care about. I think alpha is more useful for the majority audience.

@SuperSodaSea
Copy link
Member Author

I've changed the order of the options in the document to sort by alphabetical order. I keeped the order in IRendererOptions so that we can split them into multiple interfaces later as mentioned in #9112 (comment). (The member of interface will automatically sorted by Webdoc, so they are already alphabetical in the generated document.)

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Jan 30, 2023
@bigtimebuddy bigtimebuddy merged commit 8dafbe7 into dev Jan 31, 2023
@bigtimebuddy bigtimebuddy deleted the chore/renderer-options-docs branch January 31, 2023 14:41
@Zyie Zyie added the 💞 Migrate to v8 This PR needs to be migrated to v8 label Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💞 Migrate to v8 This PR needs to be migrated to v8 ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

notMultiplied for useContextAlpha renderer option missing in the docs
3 participants