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

Added Vertical Slide Transition to Carousel #32988

Closed
wants to merge 14 commits into from

Conversation

jarihant701
Copy link

@jarihant701 jarihant701 commented Feb 5, 2021

New vertical slide transition is added by adding new carousel-vertical class and added transition to translate it in Y direction

Preview: https://deploy-preview-32988--twbs-bootstrap.netlify.app/docs/5.0/components/carousel/#vertical-slide

TODO:

  • Move indicators a bit to the left?
  • Fix swipe

@XhmikosR
Copy link
Member

XhmikosR commented Feb 5, 2021

Hey, thanks for the contribution. Please try to add a docs example too.

@jarihant701
Copy link
Author

Hey, thanks for the contribution. Please try to add a docs example too.

I am new to contributing to Open Source and this is my first pull request so can you please elaborate this. If you are saying that I should include the working example that if this code is working or not so I am including a codepen link with its execution hope this helps.
https://codepen.io/jarihant701/full/wvoGaqY

@XhmikosR
Copy link
Member

XhmikosR commented Feb 6, 2021

We need a working example on https://github.com/twbs/bootstrap/blob/main/site/content/docs/5.0/components/carousel.md otherwise no one will know about this :)

Check the other examples on that page regarding how to add a new one.

@XhmikosR XhmikosR marked this pull request as draft February 6, 2021 07:22
@jarihant701
Copy link
Author

We need a working example on https://github.com/twbs/bootstrap/blob/main/site/content/docs/5.0/components/carousel.md otherwise no one will know about this :)

Check the other examples on that page regarding how to add a new one.

I have made the necessary changes. I suppose now this is good. Thank You.

@jarihant701 jarihant701 marked this pull request as ready for review February 6, 2021 07:25
@XhmikosR
Copy link
Member

XhmikosR commented Feb 6, 2021

I marked the PR as draft because it's not ready. Please clone locally and set up the tooling before marking it as ready.

@XhmikosR XhmikosR marked this pull request as draft February 6, 2021 07:26
@jarihant701
Copy link
Author

I marked the PR as draft because it's not ready. Please clone locally and set up the tooling before marking it as ready.

Sorry for such inconvenience caused but I am new to contributing to open source. But now I have watched some tutorials to see where I go wrong and fixed all the issues. If you think the latest commit is ready for review you can mark this open for review. Or if this had any more problem please let me know. Thank you for your guidance. :)

@jarihant701 jarihant701 marked this pull request as ready for review February 6, 2021 09:27
@XhmikosR
Copy link
Member

XhmikosR commented Feb 8, 2021

Now it's good, thanks!

@twbs/css-review seems to work good enough, not sure if we should mention this in the migration notes.

@jarihant701
Copy link
Author

Now it's good, thanks!

@twbs/css-review seems to work good enough, not sure if we should mention this in the migration notes.

Can you please tell me what are migration notes and if they need my contribution. Thank you for your commit😁

@mdo
Copy link
Member

mdo commented Feb 8, 2021

Would we reorient the carousel captions and controls for this? Clicking on left/right arrows that trigger something up/down doesn't feel right.

@XhmikosR
Copy link
Member

XhmikosR commented Feb 8, 2021

Agreed, it sort of feels unnatural

@jarihant701 jarihant701 requested a review from a team as a code owner February 9, 2021 11:15
@jarihant701
Copy link
Author

Would we reorient the carousel captions and controls for this? Clicking on left/right arrows that trigger something up/down doesn't feel right.

I made some changes regarding this hope this looks good.
Preview for this : https://deploy-preview-32988--twbs-bootstrap.netlify.app/docs/5.0/components/carousel/#vertical-slide

@XhmikosR
Copy link
Member

XhmikosR commented Feb 9, 2021

@jarihant701 please do this assuming you have an upstream remote

git fetch upstream
git checkout vertical-carousel-transition
git rebase -i upstream/master # and drop any unrelated patches
git push -f

width: 15px;
height: 15px;
// stylelint-disable-next-line property-disallowed-list
border-radius: 50%;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you don't use our border-radius mixin?

Copy link
Author

Choose a reason for hiding this comment

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

What is mixin? Never heard of it.
First I wrote border-radius: 50% but when I run npm run test it give me error that border-radius is not allowed so I saw in the spinner.md that they have included this CSS comment above the line and I did the same amd it worked so I keep it this way.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's not allowed for a reason :) We have internal mixins we use, see the scss/mixins folder.

Copy link
Author

Choose a reason for hiding this comment

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

I have now updated the file to use scss mixin. Hope this is what you wanted to do. Thank You.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's right, though. It should be 50%, now it's 50rem. Maybe it's a limitation of the mixin. If so, you might need to revert to your previous solution.

Copy link
Author

Choose a reason for hiding this comment

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

But I don't think that is gonna pose any problem because if we provide a radius of 50rem so it will eventually made that indicator in a circle which is our ultimate goal so reverting to previous version don't make difference.

Copy link
Member

Choose a reason for hiding this comment

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

What you have right now doesn't work

image

Copy link
Author

@jarihant701 jarihant701 Feb 9, 2021

Choose a reason for hiding this comment

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

Can you tell me the git command to revert to the previous commit i.e. commit 6f2d5c5 to undo these mixin changes. Thank you
Edit: Nevermind made new commits and reverted to previous one.

@jarihant701 jarihant701 requested review from XhmikosR and removed request for a team February 10, 2021 09:39
@GeoSot
Copy link
Member

GeoSot commented Feb 10, 2021

Guys, just a note. Carousel has js code that listens swipes too (for now only left/right)

@XhmikosR XhmikosR marked this pull request as draft February 11, 2021 06:00
@mdo
Copy link
Member

mdo commented Dec 29, 2022

Looking at this, we'd need to figure out the touch controls. Right now the staging PR has the horizontal swiping reversed—so that's also a bug that needs fixing. Going to close this one out as stale for now, but happy to re-open or see a new PR if @jarihant701 is into it.

@mdo mdo closed this Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants