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

Upgrade dom-helpers #446

Merged
merged 5 commits into from
Sep 9, 2019
Merged

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Sep 4, 2019

@taion
Copy link
Member

taion commented Sep 4, 2019

might want to update RTG first, though, since we're still pulling in dom-helpers via RTG

@TrySound
Copy link
Contributor Author

TrySound commented Sep 4, 2019

@bpas247
Copy link
Member

bpas247 commented Sep 4, 2019

hmm, strange that the CI build didn't kick off. Is this branch up to date with the master branch?

@TrySound
Copy link
Contributor Author

TrySound commented Sep 4, 2019

Yes, I pulled master before submmitting.

@bpas247
Copy link
Member

bpas247 commented Sep 4, 2019

I just ran the build locally, and it seems to be failing on this test with the following error:

Error: Uncaught AssertionError: expected 'focus-container' to include 'modal' (node_modules/chai/chai.js:239)

@TrySound TrySound force-pushed the upgrade-dom-helpers branch from f5d0fd6 to 8833780 Compare September 5, 2019 14:17
@TrySound
Copy link
Contributor Author

TrySound commented Sep 5, 2019

@bpas247 Looks like it was race condition.

@jquense
Copy link
Member

jquense commented Sep 5, 2019

yeah that happens occasionally when browser windows aren't actively focused

Copy link
Member

@bpas247 bpas247 left a comment

Choose a reason for hiding this comment

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

Seems to be working as of 8833780 👍

@bpas247
Copy link
Member

bpas247 commented Sep 5, 2019

We've changed up the CI configuration since this PR has been opened (see #448 and #439), so we'll probably need to merge the changes from master into this branch to get those required status checks to kick off properly.

@TrySound do you mind doing that? If not, I'd be more than happy to 😃

@TrySound TrySound force-pushed the upgrade-dom-helpers branch from 8833780 to d2aa1d9 Compare September 6, 2019 07:19
@taion
Copy link
Member

taion commented Sep 9, 2019

I merged this to master. Will merge once CI goes green.

@taion
Copy link
Member

taion commented Sep 9, 2019

okay... so the way we have the codecov secret set up means it's not accessible for builds from forks. bleh.

@bpas247
Copy link
Member

bpas247 commented Sep 9, 2019

okay... so the way we have the codecov secret set up means it's not accessible for builds from forks. bleh.

Any easy fix for that?

@taion
Copy link
Member

taion commented Sep 9, 2019

I don't see a way in the secret config to expose secrets to builds from forks. We might just have to wait until there's better first-party support here, and just deal with the pain for now. Not a problem in this case anyway because we don't care about coverage diffs for this change.

@taion taion merged commit 1e98bae into react-bootstrap:master Sep 9, 2019
@bpas247
Copy link
Member

bpas247 commented Sep 9, 2019

This seems related to the overall issue of not allowing token-free uploads for public repos, so hopefully they'll push a fix for it soon.

@TrySound
Copy link
Contributor Author

@taion @bpas247 is there anything blocking release?

@TrySound TrySound deleted the upgrade-dom-helpers branch September 13, 2019 20:57
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.

4 participants