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

Remove Vueper Slides & replace with Swiper #412

Merged
merged 2 commits into from
Jan 8, 2021
Merged

Conversation

heyhippari
Copy link
Contributor

@heyhippari heyhippari commented Dec 18, 2020

This removes all instances of Vueper Slides in the code, and replaces all the uses with Swiper. The reasoning is that Swiper is a better library and is officially supported on Vue 3 already.

This also removes some code duplication by creating a <swiper-section> component, which is able to take all the options of the various old usages of Vueper and replaces it everywhere. It incorporates the loading skeleton, allows you to override the section title and supports multiple sections on one page (through generating a UUID for each instance, which is then used in the root class of the component in order to differentiate it from other instances of Swiper on the page).

This change also leads to fixing a major issue with the old sliders, which prevented usage on mobile. These new sections use per-page navigation on non-mobile devices, while mobile devices use a more mobile-appropriate "free mode" to allow swiping through items easily.

@heyhippari heyhippari marked this pull request as ready for review December 19, 2020 12:25
components/HomeSection.vue Outdated Show resolved Hide resolved
components/SwiperSection.vue Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Dec 27, 2020

Codecov Report

Merging #412 (fa23495) into master (a485194) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #412      +/-   ##
=========================================
- Coverage    0.56%   0.56%   -0.01%     
=========================================
  Files          87      88       +1     
  Lines        2287    2311      +24     
  Branches      354     355       +1     
=========================================
  Hits           13      13              
- Misses       2273    2297      +24     
  Partials        1       1              
Impacted Files Coverage Δ
components/Item/RelatedItems.vue 0.00% <0.00%> (ø)
components/Layout/Backdrop.vue 0.00% <ø> (ø)
components/Layout/HomeSection.vue 0.00% <ø> (ø)
components/SwiperSection.vue 0.00% <0.00%> (ø)
pages/index.vue 0.00% <ø> (ø)
pages/person/_itemId/index.vue 0.00% <0.00%> (ø)
store/homeSection.ts 0.00% <ø> (ø)
components/Item/ItemMenu.vue 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 a485194...fa23495. Read the comment docs.

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Not sure why @camc314 comments were marked as resolved by him without further follow-ups and changes.

Aside from that, I've been testing this for some time and everything LGTM.

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Been playing more thoroughly with this and I found that the swiper's images get zoomed in when clicking on an Item in Home screen while the transition is running.

It's like the card gets destroyed and the image stops being contained in the size.

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Let's fix that in another PR

@sonarcloud
Copy link

sonarcloud bot commented Jan 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari heyhippari merged commit 2f4578d into master Jan 8, 2021
@heyhippari heyhippari deleted the remove-vueper branch January 9, 2021 00:00
@ferferga ferferga mentioned this pull request Jan 10, 2021
16 tasks
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.

4 participants