Skip to content

MINOR Upgrade Bootstrap to 4.1.2 #678

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.

Sign up for GitHub

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

Jump to bottom
Merged
lukereative merged 2 commits into from
Oct 9, 2018
Merged

MINOR Upgrade Bootstrap to 4.1.2 #678

lukereative merged 2 commits into from
Oct 9, 2018

Conversation

@robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Oct 8, 2018

@chillu chillu mentioned this pull request Oct 8, 2018
Closed
6 tasks
@robbieaverill robbieaverill self-assigned this Oct 8, 2018
@robbieaverill
Copy link
Contributor Author

robbieaverill commented Oct 8, 2018

Behat failures look real, investigating

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Oct 8, 2018

Ok the search form wasn't opening. This was caused by a change in Bootstrap: twbs/bootstrap#25806. This means that .collapse.show no longer indicates display: block in Bootstrap 4.1.x like it did in 4.0.x.

The version of Reactstrap (5.0.x) that we're using specifies these classes: https://github.com/reactstrap/reactstrap/blob/5.0.0/src/Collapse.js#L32, so the only real option for B/C support in SilverStripe 4.x is to provide a polyfill, which I'm going to push up now.

@robbieaverill robbieaverill removed their assignment Oct 8, 2018
lukereative
lukereative suggested changes Oct 9, 2018
View reviewed changes
Copy link
Contributor

@lukereative lukereative left a comment

Choose a reason for hiding this comment

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

Since this is a security fix, this change should be made against x.0 branch so it it can be merged up

lukereative
lukereative approved these changes Oct 9, 2018
View reviewed changes
Copy link
Contributor

@lukereative lukereative left a comment

Choose a reason for hiding this comment

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

I discussed this with @chillu and we agree that it'll be too risky to upgrade from 4.0.0-beta on the older releases and the risk is low as you have said. So we're just going to merge this into the latest release.

@lukereative lukereative merged commit 0944900 into silverstripe:1 Oct 9, 2018
@robbieaverill robbieaverill deleted the pulls/1.3/upgrade-bootstrap branch October 9, 2018 21:34
@robbieaverill
Copy link
Contributor Author

robbieaverill commented Oct 9, 2018

Sweet, good result all round

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@lukereative lukereative lukereative approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

affects/v4

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@robbieaverill @lukereative