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

CIF-1608 - Category Carousel Component #417

Merged
merged 12 commits into from
Oct 26, 2020
Merged

CIF-1608 - Category Carousel Component #417

merged 12 commits into from
Oct 26, 2020

Conversation

LSantha
Copy link
Collaborator

@LSantha LSantha commented Oct 6, 2020

  • created generic carousel component
  • created category carousel component
  • adjusted product carousel and related products component to extend the generic carousel component
  • added unit tests

Related PRs:

Related Issue

CIF-1608

How Has This Been Tested?

Unit tests, manual testing.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

 * created generic carousel component
 * created category carousel component
 * adjusted product carousel and related products component to extend the generic carousel component
 * added unit tests
@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #417 into master will increase coverage by 0.07%.
The diff coverage is 91.04%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #417      +/-   ##
============================================
+ Coverage     84.84%   84.92%   +0.07%     
  Complexity     1023     1023              
============================================
  Files           214      215       +1     
  Lines          5320     5387      +67     
  Branches        773      773              
============================================
+ Hits           4514     4575      +61     
- Misses          657      663       +6     
  Partials        149      149              
Flag Coverage Δ Complexity Δ
#integration 67.60% <ø> (ø) 760.00 <ø> (ø)
#jest 79.22% <ø> (ø) 0.00 <ø> (ø)
#karma 93.64% <91.04%> (-0.32%) 0.00 <0.00> (ø)
#unittests 85.91% <ø> (ø) 991.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...rce/carousel/v1/carousel/clientlibs/js/carousel.js 91.04% <91.04%> (ø) 0.00 <0.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d33ee62...92fbe51. Read the comment docs.

Copy link
Member

@herzog31 herzog31 left a comment

Choose a reason for hiding this comment

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

Please add an integration test for the new categorycarousel component, to check the markup.

@@ -12,130 +12,24 @@
*
******************************************************************************/

import Carousel from '../../../../../carousel/v1/carousel/clientlibs/js/carousel';
Copy link
Member

Choose a reason for hiding this comment

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

Is this import really necessary? The general carousel clientlib exposes the Carousel class already globally, so it should be available, since in the archetypes all clientlibs are bundled together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The product carousel gets broken without the import.

@LSantha
Copy link
Collaborator Author

LSantha commented Oct 7, 2020

@herzog31 , I've created follow-up issue for adding the new component to the component library examples and also to add IT which is based on the examples package.

@mhaack
Copy link
Contributor

mhaack commented Oct 7, 2020

I agree here with @herzog31 lets add ITs here. Yes the PR is already large but there is no need for a follow-up issue. We already have an issue ticket and tests should simply be part of it.

@mhaack mhaack added the feature New feature or request label Oct 14, 2020
LSantha and others added 3 commits October 15, 2020 10:57
 * updated examples project with category carousel
 * added integration test for category carousel
@LSantha
Copy link
Collaborator Author

LSantha commented Oct 15, 2020

I've updated the examples project and added integration test for category carousel.

 * fixed category carousel component icon in the component palette of page editor
 * extracted card template in featured category list
 * clarifications in generic carousel
 * fixed issue with JS code duplication
@alkim91 alkim91 merged commit 4e1681d into master Oct 26, 2020
@alkim91 alkim91 deleted the CIF-1608-2 branch October 26, 2020 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants