Spinner refactor #2238

HerrTopi · 2025-11-11T13:41:19Z

No description provided.

scripts/bootstrap.js


+function bootstrap() {
+  execSync(path.resolve('scripts/clean.js'), opts)


The fix is to avoid executing arbitrary shell commands built from dynamic path values via execSync. Instead, launch Node directly on the target script using execFileSync, passing the script path as an argument — this prevents shell expansion issues, avoids interpretation of spaces and special shell characters, and aligns with best practices. Specifically:

On line 68, in the bootstrap function, replace execSync(path.resolve('scripts/clean.js'), opts) with execFileSync(process.execPath, [path.resolve('scripts/clean.js')], opts). This runs the script under the Node interpreter rather than launching the script itself as a shell command.

Import execFileSync from child_process.

Optionally remove unused imports if needed (keep other code unchanged).

All changes are limited to the shown code in scripts/bootstrap.js.

github-actions · 2025-11-11T13:45:44Z

PR Preview Action v1.6.2
🚀 View preview at https://instructure.design/pr-preview/pr-2238/
Built to branch `gh-pages` at 2025-11-19 13:05 UTC. Preview will be ready when the GitHub Pages deployment is complete.

matyasf · 2025-11-19T15:46:29Z

packages/ui-spinner/src/Spinner/index.tsx

+
+  const styles = useStyle({
+    generateStyle,
+    generateComponentTheme,


You can delete this line (and generateComponentTheme.ts too)

matyasf

see my comments

matyasf · 2025-11-19T15:47:45Z

packages/ui-spinner/src/Spinner/props.ts

-  /**
-   * Render Spinner "as" another HTML element
-   */
-  as?: AsElementType


Removing this is a breaking change, please put it into the upgrade guide

matyasf · 2025-11-19T15:50:17Z

packages/ui-spinner/src/Spinner/props.ts

-   * Valid values are `0`, `none`, `auto`, `xxx-small`, `xx-small`, `x-small`,
-   * `small`, `medium`, `large`, `x-large`, `xx-large`. Apply these values via
-   * familiar CSS-like shorthand. For example: `margin="small auto large"`.
+   * Valid values are from themes. See theme.semantics.spacing. Apply these values via


This is theme specific, right? theme.semantics.spacing is not guaranteed to be this path, I'd rephrase it to "See "See theme.sharedTokens.spacing

matyasf · 2025-11-19T15:52:23Z

packages/ui-spinner/src/Spinner/README.md

-  <Spinner renderTitle="Loading" size="small" margin="0 0 0 medium" />
-  <Spinner renderTitle="Loading" margin="0 0 0 medium" />
-  <Spinner renderTitle="Loading" size="large" margin="0 0 0 medium" />
+<div style={{ display: 'flex', alignItems: 'center', gap: 'spacing.spaceMd' }}>


You cant use spacing.spaceMd here

matyasf · 2025-11-19T15:55:51Z

packages/ui-spinner/src/Spinner/styles.ts

+type StyleParams = {
+  size: SpinnerProps['size']
+  variant: SpinnerProps['variant']
+  themeOverride: SpinnerProps['themeOverride']


afaik we dont need themeOverride here, its handled by useStyle?

matyasf · 2025-11-19T15:57:58Z

packages/ui-spinner/src/Spinner/README.md

-  <Spinner renderTitle="Loading" size="small" margin="0 0 0 medium" delay={2000} />
-  <Spinner renderTitle="Loading" margin="0 0 0 medium"  delay={3000} />
-  <Spinner renderTitle="Loading" size="large" margin="0 0 0 medium"  delay={4000} />
+<div style={{ display: 'flex', alignItems: 'center', gap: 'spacing.spaceMd' }}>


You cant use this gap value here

matyasf · 2025-11-19T16:01:33Z

packages/ui-spinner/src/Spinner/styles.ts

  const spinnerSizes = {
    'x-small': {
-      width: componentTheme.xSmallSize,
-      height: componentTheme.xSmallSize
+      width: componentTheme.containerSizeXs,
+      height: componentTheme.containerSizeXs
    },
    small: {
-      width: componentTheme.smallSize,
-      height: componentTheme.smallSize
+      width: componentTheme.containerSizeSm,
+      height: componentTheme.containerSizeSm
    },
    medium: {
-      width: componentTheme.mediumSize,
-      height: componentTheme.mediumSize
+      width: componentTheme.containerSizeMd,
+      height: componentTheme.containerSizeMd
    },
    large: {
-      width: componentTheme.largeSize,
-      height: componentTheme.largeSize
+      width: componentTheme.containerSizeLg,
+      height: componentTheme.containerSizeLg
    }
  }

  const circleSizes = {
    'x-small': {
-      width: componentTheme.xSmallSize,
-      height: componentTheme.xSmallSize
+      width: componentTheme.containerSizeXs,
+      height: componentTheme.containerSizeXs
    },
    small: {
-      width: componentTheme.smallSize,
-      height: componentTheme.smallSize
+      width: componentTheme.containerSizeSm,
+      height: componentTheme.containerSizeSm
    },
    medium: {
-      width: componentTheme.mediumSize,
-      height: componentTheme.mediumSize
+      width: componentTheme.containerSizeMd,
+      height: componentTheme.containerSizeMd
    },
    large: {
-      width: componentTheme.largeSize,
-      height: componentTheme.largeSize
+      width: componentTheme.containerSizeLg,
+      height: componentTheme.containerSizeLg
    }
  }


These 2 are the same, but you are not using the spinnerSize.. tokens

matyasf · 2025-11-19T16:07:18Z

packages/ui-spinner/src/Spinner/index.tsx

+    delay,
+    renderTitle,
+    margin,
+    // elementRef,


you can delete this

matyasf · 2025-11-19T16:09:08Z

packages/ui-spinner/src/Spinner/index.tsx

-        margin={this.props.margin}
-        data-cid="Spinner"
-      >
+      <div {...passthroughProps} css={styles?.spinner} ref={ref}>


data-cid is missing from here

github-advanced-security bot found potential problems Nov 11, 2025

View reviewed changes

matyasf assigned HerrTopi Nov 13, 2025

HerrTopi changed the base branch from master to v12 November 18, 2025 16:28

HerrTopi force-pushed the spinner branch from 8009e25 to b4a3b96 Compare November 19, 2025 12:40

HerrTopi requested review from balzss and matyasf and removed request for matyasf November 19, 2025 12:40

feat(ui-spinner,ui-avatar): refactor spinner

7ef4997

HerrTopi force-pushed the spinner branch from b4a3b96 to 7ef4997 Compare November 19, 2025 13:01

matyasf reviewed Nov 19, 2025

View reviewed changes

matyasf requested changes Nov 19, 2025

View reviewed changes

@@ -24,7 +24,7 @@
              * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
              * SOFTWARE.
              */
-            const { execSync, fork } = require('child_process')
+            const { execSync, execFileSync, fork } = require('child_process')
             const path = require('path')
             const opts = { stdio: 'inherit' }
@@ -65,7 +65,7 @@
             }
             function bootstrap() {
-              execSync(path.resolve('scripts/clean.js'), opts)
+              execFileSync(process.execPath, [path.resolve('scripts/clean.js')], opts)
               buildProject()
             }


		function bootstrap() {
		execSync(path.resolve('scripts/clean.js'), opts)

Spinner refactor #2238

Are you sure you want to change the base?

Spinner refactor #2238

Uh oh!

Conversation

HerrTopi commented Nov 11, 2025

Uh oh!

Check warning

Copilot Autofix

github-actions bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Built to branch gh-pages at 2025-11-19 13:05 UTC. Preview will be ready when the GitHub Pages deployment is complete.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matyasf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Nov 11, 2025 •

edited

Loading

Built to branch `gh-pages` at 2025-11-19 13:05 UTC.
Preview will be ready when the GitHub Pages deployment is complete.