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

Bump Bootstrap 4 and 5 #1821

Merged
merged 5 commits into from Jul 25, 2022
Merged

Bump Bootstrap 4 and 5 #1821

merged 5 commits into from Jul 25, 2022

Conversation

IanDelMar
Copy link
Contributor

Description

This PR bumps

Motivation and Context

Keeping Bootstrap up to date.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I pulled my branch from develop.
  • I am submitting my pull request to develop.
  • I have resolved any conflicts merging this pull request would create.
  • I have checked that there aren't other open Pull Requests for the same update/change.
  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • (Optional) My change requires a change to the documentation.
  • (Optional) I have updated the documentation accordingly.
  • (Optional) My change requires a change to the translations.
  • (Optional) I have updated the translations accordingly.
  • composer cs:check has passed locally.
  • composer lint:php has passed locally.
  • I have read the CONTRIBUTING document.

Related Issues or Roadmap requests

Further comments

@bacoords bacoords mentioned this pull request Jul 19, 2022
17 tasks
@bacoords
Copy link
Member

One issue with the homepage widget slider: We've been using data-interval=false to disable the autoplay on the carousel (as an accessibility default. see: https://make.wordpress.org/themes/handbook/review/accessibility/required/#media).

It looks like starting in BS 5.2.0, this causes it to think we want the interval to be 0 seconds and just animate super fast.

@IanDelMar
Copy link
Contributor Author

IanDelMar commented Jul 19, 2022

You're right. data-interval=false does no longer block autoplay. An alternative could be data-bs-ride=true or data-bs-ride=false which is the default.

If set to true, autoplays the carousel after the user manually cycles the first item. If set to "carousel", autoplays the carousel on load.

From https://getbootstrap.com/docs/5.2/components/carousel/#via-data-attributes

It seems that data-interval=false has been removed because it is redundant with data-bs-ride.

@bacoords
Copy link
Member

Nice- I can verify that <div id="carouselExampleControls" class="carousel slide" data-ride="false" data-bs-ride="false"> works just fine if you want to fix the PR.

@IanDelMar
Copy link
Contributor Author

data-ride="false" is not sufficient to stop autoplay in Bootstrap 4. While it does block auto start, it autoplays after user interactions. We have to keep data-interval="false". But we can remove data-ride as its value does not have any effect after setting data-interval="false".

From the options table of the Bootstrap 4.6 docs:

Name Type Default Description
interval number 5000 The amount of time to delay between automatically cycling an item. If false, carousel will not automatically cycle.
ride string false Autoplays the carousel after the user manually cycles the first item. If set to 'carousel', autoplays the carousel on load.

@bacoords
Copy link
Member

@IanDelMar Do we know why the PHPStan analysis is failing? (I know this is not really related to the changes here).

It looks like the esc_attr( get_the_date( 'c' ) ) on https://github.com/understrap/understrap/blob/main/inc/template-tags.php#L24 is an example of it failing due to get_the_date() potentially returning false.

@IanDelMar
Copy link
Contributor Author

@IanDelMar Do we know why the PHPStan analysis is failing? (I know this is not really related to the changes here).

It looks like the esc_attr( get_the_date( 'c' ) ) on https://github.com/understrap/understrap/blob/main/inc/template-tags.php#L24 is an example of it failing due to get_the_date() potentially returning false.

I'll address the PHPStan errors in another PR. The errors are about making sure the correct argument type is used.

@bacoords bacoords merged commit a8a5ac8 into understrap:develop Jul 25, 2022
@IanDelMar IanDelMar deleted the bump-bs branch July 25, 2022 18:33
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

2 participants