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

fix(skeleton-text): fix rows reactivity in paragraph variant #1794

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

metonym
Copy link
Collaborator

@metonym metonym commented Aug 20, 2023

Fixes #1793

This refactors SkeletonText to use a more declarative approach: the Array.from method in the markup instead of a for loop inside of a reactive statement.

Copy link
Collaborator

@theetrain theetrain left a comment

Choose a reason for hiding this comment

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

This works well, though I couldn't reproduce the issue reported in #1793.

I likely misinterpreted it. Here's my attempt: https://svelte.dev/repl/078ab3788d514d3fbf9724d933e39b9b?version=4.2.0

Side note, the scripts build:docs and build:css won't work until we switch to ESM; or were you able to run them and publish recent packages some other way?

@metonym
Copy link
Collaborator Author

metonym commented Aug 23, 2023

Side note, the scripts build:docs and build:css won't work until we switch to ESM; or were you able to run them and publish recent packages some other way?

Interesting – I'm still able to run these commands. Curious as to why they require ESM?

@theetrain
Copy link
Collaborator

theetrain commented Aug 23, 2023

Not sure either, here's my output—includes yarn and node versions:

➜  carbon-components-svelte-legacy git:(fix-skeleton-text) yarn build:docs
yarn run v1.22.19
warning ../../../package.json: No license field
$ node scripts/build-docs
/.../carbon-components-svelte-legacy/scripts/build-docs.js:3
const { sveld } = require("sveld");
                  ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /.../sveld/lib/index.js from /.../carbon-components-svelte-legacy/scripts/build-docs.js not supported.
Instead change the require of index.js in /.../carbon-components-svelte-legacy/scripts/build-docs.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/.../carbon-components-svelte-legacy/scripts/build-docs.js:3:19) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.16.0

In package.json, "type": "module" or "commonjs" don't seem to influence my output.

@metonym
Copy link
Collaborator Author

metonym commented Aug 23, 2023

I'm using:

  • Yarn 1.22.19
  • Node 18.16.0

Is it possible that you were using a symlinked version of sveld when working on carbon-design-system/sveld#108?

@theetrain
Copy link
Collaborator

theetrain commented Aug 23, 2023

Yes, it was a symlink. Works now!

As for the bug, what are the reproduction steps? It seems to work fine on v0.79.0: https://svelte.dev/repl/078ab3788d514d3fbf9724d933e39b9b?version=4.2.0

@metonym
Copy link
Collaborator Author

metonym commented Aug 26, 2023

I was also no able to reproduce it in later version of Svelte. However, I feel that refactoring to avoid the for loop inside the reactive statement is worth it.

@metonym metonym merged commit bc97ce5 into master Aug 26, 2023
2 checks passed
@metonym metonym deleted the fix-skeleton-text branch August 26, 2023 17:19
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.

SkeletonText lines carry over from other instances in some cases
2 participants