Skip to content

Add static scriptName property to multiple script classes #7633

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

Merged
merged 4 commits into from
May 1, 2025

Conversation

marklundin
Copy link
Member

@marklundin marklundin commented Apr 30, 2025

Updates Scripts in scripts/esm/ to use include scriptName field as per #7593. Also updates Jsdoc examples.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a static scriptName property to multiple script classes and updates corresponding Jsdoc examples to comply with the new specification.

  • Adds static scriptName properties to various ESM script classes in both production and documentation examples.
  • Ensures consistency in how scripts are identified across the codebase.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/framework/script/script.js Updated Jsdoc examples with static scriptName properties; multiple examples for different events.
scripts/esm/xr-navigation.mjs Added static scriptName property for the XR navigation script.
scripts/esm/xr-controllers.mjs Added static scriptName property for the XR controllers script.
scripts/esm/shadow-catcher.mjs Added static scriptName property for the shadow catcher script.
scripts/esm/grid.mjs Added static scriptName property for the grid script.
scripts/esm/first-person-controller.mjs Added static scriptName property for the first person controller script.
scripts/esm/camera-frame.mjs Added static scriptName property for the camera frame script.
scripts/esm/camera-controls.mjs Added static scriptName property for the camera controls script.
examples/assets/scripts/misc/rotator.mjs Added static scriptName property for the rotator script example.
Comments suppressed due to low confidence (2)

src/framework/script/script.js:53

  • [nitpick] Multiple Jsdoc examples in this file use the same 'player-controller' value for scriptName; consider using varied names in examples to better illustrate different scenarios and reduce potential confusion.
static scriptName = 'player-controller';

src/framework/script/script.js:31

  • [nitpick] Ensure that the scriptName values in documentation examples clearly reflect the intended use cases of each script for better clarity in learning and usage.
static scriptName = 'entity-rotator';

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

Could you maybe modify one of the examples to show how to create the script using that given name.

@Maksims
Copy link
Collaborator

Maksims commented Apr 30, 2025

For script names, camelCase is used in multiple places, would be great to be consistent here.

@marklundin marklundin requested a review from Copilot May 1, 2025 08:02
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new static property, scriptName, to multiple script classes and updates associated JSDoc examples to reflect this change.

  • Updated JSDoc examples in src/framework/script/script.js with static scriptName entries
  • Added static scriptName properties for various scripts in scripts/esm (xr-navigation, xr-controllers, shadow-catcher, grid, first-person-controller, camera-frame, and camera-controls)
  • Added a static scriptName for a sample script in examples/assets/scripts/misc/rotator.mjs

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/framework/script/script.js Updated JSDoc examples with static scriptName properties (e.g., 'entityRotator', 'playerController')
scripts/esm/xr-navigation.mjs Added static scriptName ('xrNavigation') to support XR navigation
scripts/esm/xr-controllers.mjs Added static scriptName ('xrControllers') for XR controllers
scripts/esm/shadow-catcher.mjs Added static scriptName ('shadowCatcher') for the shadow catcher script
scripts/esm/grid.mjs Introduced static scriptName ('grid') for the grid script
scripts/esm/first-person-controller.mjs Added static scriptName ('firstPersonController') for the first-person controller script
scripts/esm/camera-frame.mjs Inserted static scriptName ('cameraFrame') for the camera frame script
scripts/esm/camera-controls.mjs Added static scriptName ('cameraControls') for the camera controls script
examples/assets/scripts/misc/rotator.mjs Added static scriptName ('rotator') for the sample rotator script
Comments suppressed due to low confidence (1)

src/framework/script/script.js:31

  • [nitpick] In the JSDoc examples, the added static scriptName values use different identifiers (for instance, 'entityRotator' and several instances of 'playerController'). Consider clarifying these examples either by using distinct names for each example or by adding a comment that explains these are separate sample cases.
static scriptName = 'entityRotator';

@marklundin marklundin merged commit 20da0f7 into main May 1, 2025
7 checks passed
@marklundin marklundin deleted the chore-script-name branch May 1, 2025 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants