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

Remaining to do after the jQuery removal #28342

Closed
20 of 22 tasks
XhmikosR opened this issue Feb 22, 2019 · 14 comments · Fixed by #28779
Closed
20 of 22 tasks

Remaining to do after the jQuery removal #28342

XhmikosR opened this issue Feb 22, 2019 · 14 comments · Fixed by #28779

Comments

@XhmikosR
Copy link
Member

XhmikosR commented Feb 22, 2019

@midzer
Copy link
Contributor

midzer commented Mar 11, 2020

I am trying to help and port "Replace https://codepen.io/Johann-S/pen/djJYPb or add a codepen with vanilla JS" item

This is what i got until now:
https://paste.headstrong.de/view/08ba7d0b

Too bad I can't test properly, because bootstrap object is not found (I've included https://github.com/twbs/bootstrap/blob/master-xmr-v5/dist/js/bootstrap.bundle.js)

@Johann-S
Copy link
Member

Hi @midzer, thank you for trying to tackle that, but you should wait for our first alpha release

@alpadev
Copy link
Contributor

alpadev commented Jun 18, 2020

Since the alpha has been released - some updated codepen without the jQuery dependency: https://codepen.io/-alpa-/pen/PoZWzNM

I used querySelectorAll in that case, to keep the logic from the corresponding jQuery example - although I'm not sure if that's adding some confusion. Also to keep things DRY I used a wrapper function for document.querySelector that is named $ - that is what Chrome is doing by default in case $ is undefined - but might be a bit confusing too.

Anyways I guess it'd be better if you add this to your codepen, to keep things streamlined. Feel free to change things as you see fit.

@XhmikosR
Copy link
Member Author

XhmikosR commented Nov 3, 2020

@Johann-S what's the status of this? Only 2 TODO are left :)

@Johann-S
Copy link
Member

@alpadev can you submit a PR with your CodePen link?

@alpadev
Copy link
Contributor

alpadev commented Nov 15, 2020

Sure, if you agree with the updates.

@alpadev
Copy link
Contributor

alpadev commented Nov 15, 2020

To get this list done. What would be the required information for the "Maybe explain when an element is considered visible in the scrollspy callout" todo?

Like an element is considered visible if scrollTop >= offset or that an element is considered visible if the target element has a valid getBoundingClientRect with either width or height not being 0?

@XhmikosR
Copy link
Member Author

@alpadev it's been a while but IIRC the issue was about https://getbootstrap.com/docs/4.5/components/scrollspy/#non-visible-target-elements-ignored:

Non-:visible target elements ignored

Target elements that are not :visible according to jQuery will be ignored and their corresponding nav items will never be highlighted.

@alpadev
Copy link
Contributor

alpadev commented Nov 16, 2020

Oh I see. I had a look into the code and in theory one could hide it with visibility: hidden; (and maybe height: 0; while keeping its width) and it would still work, though being barely visible :)

The logic in the code checks for both height > 0 or width > 0.

So there's nothing I can help here with I guess?

@XhmikosR
Copy link
Member Author

I don't recall TBH, I know I removed the docs part so that we move forward back then, but I don't know if the statement applies to v5 (it probably does, though).

@XhmikosR
Copy link
Member Author

So, only the scrollspy callout is left 🙂

Now, I'm not sure how to proceed with this, but if we can put it back quickly explaining when an element might not be visible and that scrollspy will ignore it, I think that would be the best solution.

PRs welcome!

@alpadev
Copy link
Contributor

alpadev commented Nov 20, 2020

@XhmikosR uhm there is some info about the visibility: https://github.com/twbs/bootstrap/blob/main/site/content/docs/5.0/components/scrollspy.md#non-visible-target-elements-ignored
It could be extended though, if you see a benefit from it.

@XhmikosR
Copy link
Member Author

Oh, true. I guess we can finally close this issue.

Thanks everyone for the help!

@AndreasA

This comment has been minimized.

@twbs twbs locked and limited conversation to collaborators Jun 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants