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

New Kirby Slides design #2949

Merged
merged 133 commits into from
Jun 14, 2023
Merged

Conversation

mark-drastrup
Copy link
Contributor

@mark-drastrup mark-drastrup commented Mar 16, 2023

Which issue does this PR close?

This PR closes #1986

What is the new behavior?

A new carousel component based on Swiper.js. This component will replace the exisiting slides component.

Does this PR introduce a breaking change?

  • Yes
  • No

Are there any additional context?

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Determine if your changes are a fix, feature or breaking-change, and add the matching label to your PR. If it is tooling, dependency updates or similar, add ignore-for-release.
  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be merged to develop by Team Kirby.

@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel March 16, 2023 12:07 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel March 16, 2023 12:19 Inactive
@mark-drastrup mark-drastrup force-pushed the enhancement/1986-desktop-ready-carousel branch from 9a2e349 to 2529404 Compare March 28, 2023 09:01
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel March 28, 2023 09:08 Inactive
@mark-drastrup mark-drastrup force-pushed the enhancement/1986-desktop-ready-carousel branch from 8763901 to 1ae8311 Compare March 29, 2023 09:57
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel March 29, 2023 10:07 Inactive
@mark-drastrup mark-drastrup force-pushed the enhancement/1986-desktop-ready-carousel branch 2 times, most recently from 4b27646 to 40fbfbb Compare April 20, 2023 08:37
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 20, 2023 08:45 Inactive
@mark-drastrup mark-drastrup changed the title New Carousel Kirby Carousel Apr 21, 2023
@mark-drastrup mark-drastrup added the feature Add this PR to the changelog as a feature label Apr 21, 2023
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 21, 2023 05:53 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 21, 2023 06:57 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 21, 2023 07:41 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 24, 2023 08:57 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 24, 2023 10:38 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 24, 2023 11:46 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 24, 2023 12:05 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 24, 2023 12:56 Inactive
@mark-drastrup mark-drastrup marked this pull request as ready for review April 25, 2023 04:37
@kodeaben
Copy link
Contributor

Comments:

  1. The "Kirby page width" on the cookbook. I can't see changing the value in the dropdown makes any difference
  2. There is only a mobile example in the cookbook. Aren't we going to use this on desktop? The examples in the issue looks like desktop
  3. In tests, there is only tested with one set of configurations. Should we test with more configurations?

@kodeaben
Copy link
Contributor

This module isn't exported through KirbyModule I think it should be. @RasmusKjeldgaard ?

@mark-drastrup mark-drastrup force-pushed the enhancement/1986-desktop-ready-carousel branch from fd0b598 to 61df7e0 Compare April 26, 2023 05:32
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 26, 2023 05:38 Inactive
libs/core/src/scss/_global-styles.scss Outdated Show resolved Hide resolved
border-radius: 2px;
}

div.pagination span.swiper-pagination-bullet:last-of-type {
Copy link
Contributor

Choose a reason for hiding this comment

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

Whis looks more like it should be placed in the carrousel component styling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree, but for some reason I cannot make it work if I place the styling in carousel.component.scss. It's not because the styling is overridden, but more like it is never applied to the bullet span elementens. Do you have any ideas what the problem could be @RasmusKjeldgaard?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mark-drastrup this is due to Angular style encapsulation. Your span selector will be augmented with _ngcontent-xxx attribute which the bullets don't have as they are added to the dom by swiperjs. This can be fixed by using ::ng-deep on the selector in the component scss - as in:
.pagination ::ng-deep span.swiper-pagination-bullet:last-of-type { ... }

libs/designsystem/carousel/src/carousel.component.scss Outdated Show resolved Hide resolved
libs/designsystem/carousel/src/carousel.component.spec.ts Outdated Show resolved Hide resolved
libs/designsystem/carousel/src/carousel.component.spec.ts Outdated Show resolved Hide resolved
libs/designsystem/carousel/src/carousel.component.ts Outdated Show resolved Hide resolved
@mark-drastrup mark-drastrup force-pushed the enhancement/1986-desktop-ready-carousel branch from 29e604f to 6587fd2 Compare April 26, 2023 13:05
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel April 26, 2023 13:12 Inactive
@kodeaben
Copy link
Contributor

  1. In the cookbook, The example is placed in a container (full size), which introduces scroll:
    scroll

  2. When I use the "Activate slide no 4" (full size), it centeres slide 5, but I would expect it would center slide 4.

  3. Import using KirbyModule works :)

@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel June 13, 2023 14:38 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel June 14, 2023 06:38 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel June 14, 2023 09:01 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel June 14, 2023 09:11 Inactive
Copy link
Collaborator

@RasmusKjeldgaard RasmusKjeldgaard left a comment

Choose a reason for hiding this comment

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

Only two docs-related things from me on this one!

@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel June 14, 2023 10:53 Inactive
@jakobe jakobe enabled auto-merge (squash) June 14, 2023 11:25
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel June 14, 2023 11:28 Inactive
@github-actions github-actions bot temporarily deployed to pr-1986-desktop-ready-carousel June 14, 2023 11:28 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Add this PR to the changelog as a feature
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Enhancement] Refactor Kirby-Slide (Desktop Ready?)
4 participants