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 AccordionSkeleton and SkeletonText components #178

Merged
merged 12 commits into from
Feb 13, 2019

Conversation

sabov
Copy link
Contributor

@sabov sabov commented Feb 7, 2019

Issue #167

Adds CvAccordionSkeleton and CvSkeletonText components. Updates CvAccordion story.

Changelog

New

  • AccordionSkeleton
  • SkeletonText

Changed

  • CvAccordion story

Preview

skeleton

Copy link
Member

@lee-chase lee-chase left a comment

Choose a reason for hiding this comment

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

Can we make the skeletons 'functional' components?

https://vuejs.org/v2/guide/render-function.html#Functional-Components

If CvSeketonText is for public use it needs a story and markdown file. If it is only for internal consumption then please prefix the filename with an _ otherwise it is picked up by components/index.js

@sabov
Copy link
Contributor Author

sabov commented Feb 12, 2019

Hey @lee-chase, I was able to turn CvAccordionItemSkeleton to a functional component but it doesn't work for CvAccordionSkeleton. I'm getting this error when I add functional keyword to the template:

vue.esm.js:628 [Vue warn]: Unknown custom element: <cv-accordion-item-skeleton> - did you register the component correctly? For recursive components, make sure to provide the "name" option.

found in
---> <Anonymous>
       <Root>

Do you have any idea why?

@lee-chase
Copy link
Member

Looks to be a known issue @sabov

vuejs/vue#7492

This works or we can use render functions. Not sure it's worth the benefit. What do you think?

<template functional>
  <ul class="bx--accordion bx--skeleton">
    <component :is="injections.components.CvAccordionItemSkeleton" open>
      <component :is="injections.components.CvSkeletonText"/>
      <component :is="injections.components.CvSkeletonText"/>
      <component :is="injections.components.CvSkeletonText"/>
    </component>

    <component :is="injections.components.CvAccordionItemSkeleton"/>
    <component :is="injections.components.CvAccordionItemSkeleton"/>
    <component :is="injections.components.CvAccordionItemSkeleton"/>
  </ul>
</template>

<script>
import CvSkeletonText from '../cv-skeleton-text/cv-skeleton-text';
import CvAccordionItemSkeleton from './_cv-accordion-item-skeleton';

export default {
  name: 'CvAccordionSkeleton',
  inject: {
    components: {
      default: {
        CvSkeletonText,
        CvAccordionItemSkeleton,
      },
    },
  },
};
</script>

@lee-chase
Copy link
Member

Could make a version soon
vuejs/vue#8143

@sabov
Copy link
Contributor Author

sabov commented Feb 13, 2019

Hey @lee-chase, I'm not sure about the benefit as well. I would vote for using functional only in dead simple components without any dependencies.

@lee-chase
Copy link
Member

OK @sabov we can return to functional if the above issue gets fixed.

Is this ready to merge, or do you need to undo any changes made for functional components?

@sabov
Copy link
Contributor Author

sabov commented Feb 13, 2019

It's good to go. I've used functional only in _cv-accordion-item-skeleton.vue and it works well.

@lee-chase
Copy link
Member

It's good to go. I've used functional only in _cv-accordion-item-skeleton.vue and it works well.

Revert functional for now in case of other issues.

@lee-chase lee-chase merged commit 46ac1cf into carbon-design-system:master Feb 13, 2019
@sabov sabov deleted the accordion-skeleton branch February 13, 2019 10:06
dcwarwick pushed a commit to dcwarwick/carbon-components-vue that referenced this pull request Mar 18, 2019
…n-system#178)

* feat: add AccordionSkeleton and SkeletonText components

* feat: update CvSkeletonText component

* chore: turned CvAccordionSkeleton to a functional component

* chore: turn CvAccordionItemcSkeleton to a functional component

* chore: use correct notation for functional components

* chore: revert functional in CvAccordionItemSkeleton
dcwarwick pushed a commit to dcwarwick/carbon-components-vue that referenced this pull request Mar 18, 2019
…n-system#178)

* feat: add AccordionSkeleton and SkeletonText components

* feat: update CvSkeletonText component

* chore: turned CvAccordionSkeleton to a functional component

* chore: turn CvAccordionItemcSkeleton to a functional component

* chore: use correct notation for functional components

* chore: revert functional in CvAccordionItemSkeleton
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.

2 participants