refactor: improved provide inject by using a single symbol #434

Tofandel · 2024-11-29T03:09:53Z

fix(reactivity): use provide to register slides instead of traversing nodes (fixes #433, #350)
fix(reactivity): use computed and watchers where possible instead of manual updates
fix(reactivity): update vModel as soon as we are sliding instead of after the transition (fixes #428)
fix(carousel): update sizes during animations (fixes #338)
~~fix(carousel): in vertical mode with height auto, calculate the height of the slides automatically~~
fix(accessibility): make elements inside cloned slides non focusable and increase size of pagination buttons (fixes #346)
fix(accessibility): add arrow keys support when carousel is focused (via tab)
fix(performance): only clone itemsToShow elements instead of all the slides
chore(types): improve typings & add tsc during tests & export properly defined components
chore(package): update scripts in package json with new workflows and make package manager agnostic
fix(package): fix order of browser, require and import
chore(github): add test action for PRs
feat(dev): add development playground

This is quite the big refactor but I couldn't really split this into multiple PRs.
There is no breaking change on the api side.
Feel free to use the new playground to experiment (we could also add it to the docs)

fix(reactivity): use provide to register slides instead of traversing nodes fix(reactivity): use computed and watchers where possible instead of manual updates fix(reactivity): update vModel as soon as we are sliding instead of after the transition fix(carousel): update sizes during animations fix(carousel): in vertical with height auto, calculate the height of the slides automatically fix(accessibility): make elements inside cloned slides non focusable and increase size of pagination buttons fix(performance): only clone itemsToShow elements instead of all the slides chore(types): improve typings & add tsc during tests chore(package): update scripts in package json with new workflows and make package manager agnostic fix(package): fix order of browser, require and import chore(github): add test action for PRs feat(dev): add development playground

ismail9k · 2024-11-29T07:42:51Z

Hi @Tofandel,

Thank you so much for this extensive PR and all the effort you’ve put into it! 🎉 I can see how much thought went into these fixes and improvements, and I appreciate the attention to reactivity, and performance.

I’ll review the changes in detail and experiment with the new playground soon. However, I want to flag that some of this work may overlap or conflict with updates I’m currently working on in another branch. I’ll take the necessary steps to reconcile those changes as part of the review process.

Thanks again for contributing so much value. I’ll do my best to handle this in a way that aligns with the project’s goals.

Tofandel · 2024-11-29T10:27:24Z

Thanks, I'm sure there could be some conflicts because it's extensive changes, but I still tried to keep it to a minimum while covering as much as I could

Don't hesitate to ping me on the code if you need some explanations

ismail9k

Thank you for your hard work on this PR! I’ve gone through the changes (although it was quite hard due to the PR size), but I believe your changes are a great addition and in the right direction. I’ve added some feedback.

src/components/Carousel.ts

playground/App.vue

ismail9k · 2024-11-29T09:44:40Z

src/components/Icon.ts

+    }
+  },
+  setup(props: IconProps) {
+    const carousel = inject(injectCarousel, null)


Let's keep constant across the different component

Suggested change

const carousel = inject(injectCarousel, null)

const carousel = inject(injectCarousel)

if (!carousel) {

return null // Don't render, let vue warn about the missing provide

}

In this case, this was so this component could be used standalone outside of a carousel, since this is an exported component and the previous behavior was to allow that, if we don't provide a default value, vue will warn without the injected value and not render, so this would be a breaking change

Users could be using this for example to have their own controls outside of the carousel while keeping the lib icons

This also gave me an idea for an improvement, we could make Pagination and Navigation get a ref to the carousel and get the injection from the ref, to allow them to be outside of a carousel but still control it as well, for a future PR

I added tests for this component

src/components/Icon.ts

src/utils/getSlidesVNodes.ts

src/components/Carousel.ts

package.json

Co-authored-by: Ismail9k <dev@ismail9k.com>

package.json

Co-authored-by: Ismail9k <dev@ismail9k.com>

Tofandel

Forgot to submit my comments, dammit

src/components/Carousel.ts

Tofandel · 2024-11-29T11:16:39Z

src/components/Icon.ts

+    }
+  },
+  setup(props: IconProps) {
+    const carousel = inject(injectCarousel, null)


In this case, this was so this component could be used standalone outside of a carousel, since this is an exported component and the previous behavior was to allow that, if we don't provide a default value, vue will warn without the injected value and not render, so this would be a breaking change

Users could be using this for example to have their own controls outside of the carousel while keeping the lib icons

This also gave me an idea for an improvement, we could make Pagination and Navigation get a ref to the carousel and get the injection from the ref, to allow them to be outside of a carousel but still control it as well, for a future PR

src/utils/getSlidesVNodes.ts

src/components/Carousel.ts

Tofandel · 2024-11-29T12:44:05Z

playground/App.vue

        :gap="10"
-        :height="height || 'auto'"
+        :height="parseInt(height) || 'auto'"


I added something to handle this correctly on the component side lower (|| parseInt(config.height).toString() === config.height),

This allows for other values like 30vh or 10%

Tofandel · 2024-11-29T12:45:04Z

src/components/Carousel.ts


      // Calculate size based on orientation
+      const { width, height } = viewport.value.getBoundingClientRect()


The idea was that if we have a number height specified, we already know the height in pixels, so we don't need to call getBoundingClientRect which is a call that can force a reflow, so more performant

The multiplier there was part of the animation check, without it the width inside a transform will not be properly calculated (you can see this in the playground during the animation)

Tofandel · 2024-11-29T12:53:42Z

I forgot to submit my comment on your review (this interface is always confusing with just the "pending" labels), which is why you didn't see them before you updated the files

I reverted some of your changes accordingly

The change was found to cause issues during static site generation processes, particularly with frameworks like VitePress.

ismail9k requested changes Nov 29, 2024

View reviewed changes

ismail9k reviewed Nov 29, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

Update src/components/Carousel.ts

80a2721

Co-authored-by: Ismail9k <dev@ismail9k.com>

ismail9k reviewed Nov 29, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

Tofandel and others added 5 commits November 29, 2024 12:21

Update package.json

4f96545

Co-authored-by: Ismail9k <dev@ismail9k.com>

Revert auto height calculation and fix reviews

950f03f

feat: final updates before merge

f7f0802

Update src/components/Icon.ts

20d05ae

Update src/components/Icon.ts

522c85b

ismail9k self-requested a review November 29, 2024 12:35

ismail9k self-assigned this Nov 29, 2024

ismail9k approved these changes Nov 29, 2024

View reviewed changes

Tofandel force-pushed the master branch from 24738e3 to b45555f Compare November 29, 2024 12:50

Tofandel commented Nov 29, 2024

View reviewed changes

Tofandel force-pushed the master branch 3 times, most recently from 913f099 to ae1bcb2 Compare November 29, 2024 13:02

Tofandel added 2 commits November 29, 2024 15:55

Add tests for icon component

f155d93

Fix duplicated deps

2b377a5

Tofandel force-pushed the master branch from 6a1cada to 2b377a5 Compare November 29, 2024 14:55

ismail9k merged commit 22b1e65 into ismail9k:master Dec 1, 2024
2 checks passed

Tofandel mentioned this pull request Dec 3, 2024

How to use Slide's isClone property? #360

Closed

ismail9k mentioned this pull request Dec 5, 2024

Documentation seemingly incorrect for initDefaultConfig #437

Closed

Tofandel mentioned this pull request Dec 6, 2024

Allow ignoring animations #441

Merged

ismail9k added a commit that referenced this pull request Dec 8, 2024

revert(#434): use getSlidesVNodes utility to extract slides to be clone

a1c17b3

The change was found to cause issues during static site generation processes, particularly with frameworks like VitePress.

Tofandel mentioned this pull request Dec 8, 2024

revert(#434): use getSlidesVNodes utility to extract slides to be clone #443

Closed

ismail9k mentioned this pull request Dec 19, 2024

Webpack 5.97.1 started to load carousel.js since 0.9.0 instead of carousel.es.js #457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: improved provide inject by using a single symbol #434

refactor: improved provide inject by using a single symbol #434

Tofandel commented Nov 29, 2024 •

edited

Loading

ismail9k commented Nov 29, 2024

Tofandel commented Nov 29, 2024

ismail9k left a comment

ismail9k Nov 29, 2024

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024

Tofandel left a comment

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel commented Nov 29, 2024 •

edited

Loading

-    const carousel = inject(injectCarousel, null)
+    const carousel = inject(injectCarousel)
+    if (!carousel) {
+      return null // Don't render, let vue warn about the missing provide
+    }


		// Calculate size based on orientation
		const { width, height } = viewport.value.getBoundingClientRect()

refactor: improved provide inject by using a single symbol #434

refactor: improved provide inject by using a single symbol #434

Conversation

Tofandel commented Nov 29, 2024 • edited Loading

ismail9k commented Nov 29, 2024

Tofandel commented Nov 29, 2024

ismail9k left a comment

Choose a reason for hiding this comment

ismail9k Nov 29, 2024

Choose a reason for hiding this comment

Tofandel Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Tofandel Nov 29, 2024

Choose a reason for hiding this comment

Tofandel left a comment

Choose a reason for hiding this comment

Tofandel Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Tofandel Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Tofandel Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Tofandel Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Tofandel commented Nov 29, 2024 • edited Loading

Tofandel commented Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel Nov 29, 2024 •

edited

Loading

Tofandel commented Nov 29, 2024 •

edited

Loading