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

Update to bootstrap 5.2 #13996

Merged
merged 9 commits into from Aug 22, 2022
Merged

Update to bootstrap 5.2 #13996

merged 9 commits into from Aug 22, 2022

Conversation

MoonE
Copy link
Contributor

@MoonE MoonE commented Aug 16, 2022

I'll provide a separate pull request to remove jquery for some parts shortly.
jQuery is removed except a few pages.

@github-actions
Copy link

📦 Preview the website for this branch here: https://deploy-preview-13996--ol-site.netlify.app/.

@MoonE MoonE marked this pull request as draft August 16, 2022 19:20
@tschaub
Copy link
Member

tschaub commented Aug 16, 2022

You may be working on this, but it looks like there are some spacing issues around the news item.

With these changes:

Before:

@jipexu
Copy link
Contributor

jipexu commented Aug 16, 2022

Yes it was very necessary ! @MoonE good job !

  • in the deploy the dropdown of thedropdown of the code menu is misaligned ...

@jipexu
Copy link
Contributor

jipexu commented Aug 16, 2022

@MoonE
in this example
https://deploy-preview-13996--ol-site.netlify.app/en/latest/examples/button-title.html
if we remove jquery we have to call the tooltip in another way ...

@MoonE
Copy link
Contributor Author

MoonE commented Aug 16, 2022

Thanks, @jipexu, I'm aware of that. I'll provide a new PR to remove jquery once this is merged.

@jipexu
Copy link
Contributor

jipexu commented Aug 16, 2022

ah ok good ! update bs is always a lot of work ...

examples/index.html Outdated Show resolved Hide resolved
@tschaub
Copy link
Member

tschaub commented Aug 16, 2022

I think I like the nav links without the underline. It looks like this change adds them on hover/active/focus:

image

What do you think?

@tschaub
Copy link
Member

tschaub commented Aug 16, 2022

This is looking really good. Thanks for all your work on the upgrade, @MoonE!

@MoonE MoonE force-pushed the bootstrap-5 branch 4 times, most recently from 2b5a55c to 8d284e5 Compare August 21, 2022 17:31
@MoonE
Copy link
Contributor Author

MoonE commented Aug 21, 2022

@MoonE in this example https://deploy-preview-13996--ol-site.netlify.app/en/latest/examples/button-title.html if we remove jquery we have to call the tooltip in another way ...

@jipexu Sorry, I may have misundertstood. Even with jQueryand bootstrap 5 these tooltips no longer worked, though there are the normal browser tooltips from the tooltip attribute.

I udpated the examples to work again with bootstrap 5, hopefully everything is working now.

@MoonE MoonE marked this pull request as ready for review August 21, 2022 17:45
@jipexu
Copy link
Contributor

jipexu commented Aug 21, 2022

@MoonE in this example https://deploy-preview-13996--ol-site.netlify.app/en/latest/examples/button-title.html if we remove jquery we have to call the tooltip in another way ...

@jipexu Sorry, I may have misundertstood. Even with jQueryand bootstrap 5 these tooltips no longer worked, though there are the normal browser tooltips from the tooltip attribute.

I udpated the examples to work again with bootstrap 5, hopefully everything is working now.

@MoonE
yes because it is 2 problems :
if we remove jquery we can not use $ in our examples
if we dont remove jquery doesnt work too because the way to call tooltip has changed in bs5 ...

yes i see your change are ok good job ! this example work again in deploy https://deploy-preview-13996--ol-site.netlify.app/en/latest/examples/kml-timezones.html

@jipexu
Copy link
Contributor

jipexu commented Aug 22, 2022

@MoonE
concerning the change of the case of utf-8 and DOCTYPE in the examples
as the meta charset is case insensitive we can use both
but for the doctype who is also case insensitive in html5, i will let in upper case for a better compliance with xhtml or polyglot documents ? ....
it’s just a thought i dont know what is better ...

@MoonE
Copy link
Contributor Author

MoonE commented Aug 22, 2022

@jipexu Honestly it doesn't really matter, either works without problems. I simply prefer it to be consistent.

@@ -41,6 +41,10 @@ Open the `index.html` file in a text editor. It should look something like this
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Quick Start</title>
<style>
#map { height: 400px; }
Copy link
Member

Choose a reason for hiding this comment

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

This conflicts with the rule in the style.css. Not important to change, but I also don’t think it needs to be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, I didn't notice the css imported from the js file.

Copy link
Member

@tschaub tschaub left a comment

Choose a reason for hiding this comment

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

Thanks for the huge effort, @MoonE!

@MoonE MoonE merged commit 62914bb into openlayers:main Aug 22, 2022
@MoonE MoonE deleted the bootstrap-5 branch August 22, 2022 17:31
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.

None yet

3 participants