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

feat: add carousel component #227

Merged
merged 26 commits into from
Jan 8, 2024
Merged

Conversation

wasimTQ
Copy link
Contributor

@wasimTQ wasimTQ commented Dec 29, 2023

  • Added caroused for default styles
  • Added example for orientation vertical

fixes #226

I need help on how to add it to the table of contents

Still there are some examples that needs to be done. And also it has to be copied to new york styling as well

@wasimTQ

This comment was marked as resolved.

@sadeghbarati

This comment was marked as off-topic.

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 29, 2023

@sadeghbarati Pretty much everything is done. I just have to know how I can generate the .json files.

Can you explain how can I do that?

Also please let me know if I did it all correct. Thank you

@wasimTQ wasimTQ changed the title Feature/carousel feat: Carousel component Dec 29, 2023
@wasimTQ wasimTQ changed the title feat: Carousel component feat: add carousel component Dec 29, 2023
@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Dec 29, 2023

Carousel.-.shadcn_vue.-.Google.Chrome.2023-12-29.20-29-42.mp4

@wasimTQ

This comment was marked as resolved.

@sadeghbarati

This comment was marked as resolved.

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 30, 2023

Also I'm looking to make the carousel a focusable element for easier keyboard interactions.

I've looked in the internet. tabindex="0" is the answer I got. But the problem is it don't unfocus on tab click once it's been focused

@sadeghbarati
Copy link
Collaborator

We should change ComponentPreview tabindex when it's about Carousel demos

For focusable carousel just add tabindex 0 to Carousel.vue

@sadeghbarati
Copy link
Collaborator

For building registry you can run build:registry in apps/www directory, it's optional but you can ignore it and let Zernonia build it before the release

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 30, 2023

@sadeghbarati I think I'm pretty much done. Can you please review it once more and let me know when you're free?

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Dec 30, 2023

Only one thing which I doubt to include it in Carousel.vue or not

defineExpose({
  carouselRef: carouselArgs or carouselApi
})

So the user can access the Carousel API in every possible way in Vue

  • Ref on Component
  • @init-api emit
  • useCarousel access Carousel API in component's descendants
  • slotProps

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 30, 2023

It's already accessible through slotProps. I'll check if setting defineExpose is important.

I think it's a nice to have. I'll add it

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Dec 30, 2023

@sadeghbarati I've made all the changes. You can check it when you're free and let me know

@sadeghbarati
Copy link
Collaborator

#227 (comment)

Sometimes I still have this issue but it will be resolved after embla 8.0.0-rc18, Thanks to the embla-carousel author
Let's wait until that version gets published after that we can merge this

@zernonia
Copy link
Contributor

zernonia commented Jan 2, 2024

#227 (comment)

Sometimes I still have this issue but it will be resolved after embla 8.0.0-rc18, Thanks to the embla-carousel author Let's wait until that version gets published after that we can merge this

@sadeghbarati do you have the relevant ticket to the issue you mentioned? 😁

@sadeghbarati
Copy link
Collaborator

Hey, Yes here

davidjerleke/embla-carousel#672

@zernonia
Copy link
Contributor

zernonia commented Jan 2, 2024

Thanks @sadeghbarati ..

Something weird in the build:registry, I can't seems to run them lol

/Users/zernonia/Desktop/RadixVue/shadcn-vue/node_modules/.pnpm/@vue+compiler-sfc@3.3.7/node_modules/@vue/compiler-sfc/dist/compiler-sfc.cjs.js:15656
    throw new Error(
          ^

Error: [@vue/compiler-sfc] Failed to resolve import source "./interface".

anonymous.vue
4  |  import { cn } from '@/lib/utils'
5  |  
6  |  const props = withDefaults(defineProps<CarouselProps>(), {
   |                                         ^^^^^^^^^^^^^
7  |    orientation: 'horizontal',
8  |  })

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jan 2, 2024

Sadly Vue doesn't support Complex Types (entire props object) in SFC file, I thought I would solve this issue by defining opts objects key types one by one like, but it's still not working

export interface CarouselProps {
  opts?: {
    active: CarouselOptions['active']
    align: CarouselOptions['align']
    axis: CarouselOptions['axis']
    breakpoints: CarouselOptions['breakpoints']
    container: CarouselOptions['container']
    containScroll: CarouselOptions['containScroll']
    direction: CarouselOptions['direction']
    dragFree: CarouselOptions['dragFree']
    dragThreshold: CarouselOptions['dragThreshold']
    duration: CarouselOptions['duration']
    inViewThreshold: CarouselOptions['inViewThreshold']
    loop: CarouselOptions['loop']
    skipSnaps: CarouselOptions['skipSnaps']
    slides: CarouselOptions['slides']
    slidesToScroll: CarouselOptions['slidesToScroll']
    startIndex: CarouselOptions['startIndex']
    watchDrag: CarouselOptions['watchDrag']
    watchResize: CarouselOptions['watchResize']
    watchSlides: CarouselOptions['watchSlides']
  }
  ...
}

cause EmblaOptionsType (OptionsType) is using CreateOptionsType which is using extends

CarouselOptions types definitions

export type EmblaOptionsType = Partial<OptionsType>;

// here is where trouble started, this types is complex
export type OptionsType = CreateOptionsType<{...}> 

// OptionType also extends other type
export type CreateOptionsType<Type extends LooseOptionsType> = Type & {
    active: boolean;
    breakpoints: {
        [key: string]: Omit<Partial<CreateOptionsType<Type>>, 'breakpoints'>;
    };
};

Hands down to Vue Reactivity system, it is best, but their TypeScript support is in mid level 😶‍🌫️

@zernonia
Copy link
Contributor

zernonia commented Jan 2, 2024

Yeah. I wish the type system for Vue would be improved soon! Maybe lets try this? https://github.com/so1ve/vue.ts/tree/main/packages/complex-types

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jan 2, 2024

I tried to exclude carousel files from vue-tsc typecheck but vue-tsc ignored exclude property in tsconfig.registry.json

vuejs/language-tools#3144

{
  "extends": "@vue/tsconfig/tsconfig.json",
  "compilerOptions": {
    "moduleResolution": "Node",
    "baseUrl": ".",
    "paths": {
      "@/*": ["./src/*"]
    },
    "declaration": false
  },
  "include": ["src/lib/**/*"],
  "exclude": ["node_modules", "src/lib/registry/**/example/**/*", "src/lib/registry/default/ui/carousel/*", "src/lib/registry/new-york/ui/carousel/*"]
}

in this case we should build registry without vue-tsc typecheck

@sadeghbarati
Copy link
Collaborator

@wasimTQ can you update embla-carousel-vue to latest version, thanks

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Jan 6, 2024

@sadeghbarati It's done

@wasimTQ
Copy link
Contributor Author

wasimTQ commented Jan 8, 2024

@sadeghbarati I've made the changes.

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jan 8, 2024

@zernonia

Problem is not complex types in fact if there was complex type error we can't even open carousel.md path in the localhost and should see vite error overlay on screen

The problem is @vue/compiler-sfc can't resolve types in registry build, I remember "Dunqing" had this issue in packages/cli too which Idk how he solved it


import type { CarouselEmits, CarouselProps, WithClassAsProps } from './interface'

const props = withDefaults(defineProps<CarouselProps & WithClassAsProps>(), {
 orientation: 'horizontal',
})

[@vue/compiler-sfc] Failed to resolve import source "./interface"


import type { HTMLAttributes, Ref } from 'vue'
import type {
  EmblaCarouselType as CarouselApi,
  EmblaOptionsType as CarouselOptions,
  EmblaPluginType as CarouselPlugin,
} from 'embla-carousel'

const props = withDefaults(defineProps<{
  opts?: CarouselOptions | Ref<CarouselOptions>
  plugins?: CarouselPlugin[] | Ref<CarouselPlugin[]>
  orientation?: 'horizontal' | 'vertical'
  class?: HTMLAttributes['class']
}>(), {
  orientation: 'horizontal',
})

@zernonia
Copy link
Contributor

zernonia commented Jan 8, 2024

Thanks @sadeghbarati . Found the issue with @vue/compiler-sfc and fixed it. Now generating the registry correctly and as expected.

Copy link
Contributor

@zernonia zernonia left a comment

Choose a reason for hiding this comment

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

Awesome job @wasimTQ @sadeghbarati ! You 2 absolutely nailed it!!
Thanks for the care for details, and the thorough implementation! 💚

LGTM and let's get this baby in!

@zernonia zernonia merged commit 97c7417 into radix-vue:dev Jan 8, 2024
1 check failed
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.

[Feature]: Carousel component (Embala)
4 participants