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

chore(package): update to react 16 #2131

Merged
merged 20 commits into from
Oct 10, 2017
Merged

chore(package): update to react 16 #2131

merged 20 commits into from
Oct 10, 2017

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Sep 27, 2017

@bcarroll22
Copy link

React 16 also shows warnings when refs are passed to Stateless Functional Components, which causes problems for a lot of core SUI components.

Not sure if the fixes for those things should come in a different pull request, or if this one should be treated as a place to host all fixes to get rid of those warnings as well, but it would be nice to see them go away.

@rijk
Copy link
Member

rijk commented Sep 29, 2017

Are there also plans to implement this for Portal?

@layershifter
Copy link
Member Author

Yes, but the priority task is update deps and find broken things when we able run react 16 and enzyme 3.

@levithomason
Copy link
Member

Also, we cannot abandon Portal while there are still users using React <16. Not sure what the migration path will look like there right now.

@soederpop
Copy link
Contributor

@layershifter the chai-enzyme package has been updated to react 16.0.0

I am going to try and see if I can get securingsincity/react-ace#268 and securingsincity/react-ace#268 merged.

I've never contributed or worked on this code base, but I'm curious how difficult it would be for me to produce a react-16 compatible build of semantic-ui-react to test with?

Since react-ace is a dependency of the documentation site, I feel like it might be ok to start

@layershifter
Copy link
Member Author

I've pushed some changes yesterday and will continue this work today.

react-ace

We use it on the docs site, it's not dependency of SUIR. But, as we use satisfied, it blocked test runs. I've temporialy disabled it.

wrapper.childAt(0).key().should.equal('.$first')

// TODO: Re-enable in future
// wrapper.childAt(0).key().should.equal('.$first')
Copy link
Member Author

Choose a reason for hiding this comment

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

A very strange thing, key() is null there. looks like bug in enzyme.

.at(1)
.simulate('click')

const icons = wrapper.find('RatingIcon')
Copy link
Member Author

Choose a reason for hiding this comment

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

.should.not.be.checked()

wrapper.simulate('click')
wrapper
Copy link
Member Author

Choose a reason for hiding this comment

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

Chaining is broken 😭

.should.have.prop('active', true)
// arrow up, selection moved to last item
domEvent.keyDown(document, { key: 'ArrowUp' })
wrapper.should.have.state('selectedIndex', (categoryLength * categoryResultsLength) - 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't happy about this change, but it's impossible to test it now because children are not updated correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted them,

@layershifter
Copy link
Member Author

layershifter commented Oct 5, 2017

Waiting for:
enzymejs/enzyme#1221
enzymejs/enzyme#1229

It's impossible to correctly update all Dropdown tests.


Update, For mount, updates are sometimes required when they weren't before

@layershifter layershifter mentioned this pull request Oct 5, 2017
@@ -131,13 +131,13 @@ describe('Sticky', () => {
mockPositions({ bottomOffset: 10, height: 50 })
wrapperMount(<Sticky {...positions} context={contextEl} onStick={onStick} onUnstick={onUnStick} />)

wrapper.childAt(1).should.have.style('position', 'fixed')
wrapper.childAt(0).childAt(1).should.have.style('position', 'fixed')
Copy link
Member Author

Choose a reason for hiding this comment

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

@codecov-io
Copy link

codecov-io commented Oct 5, 2017

Codecov Report

Merging #2131 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2131      +/-   ##
==========================================
- Coverage   99.76%   99.73%   -0.04%     
==========================================
  Files         151      151              
  Lines        2606     2606              
==========================================
- Hits         2600     2599       -1     
- Misses          6        7       +1
Impacted Files Coverage Δ
src/modules/Search/Search.js 99.46% <0%> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee931d2...b5cdff5. Read the comment docs.

@layershifter
Copy link
Member Author

Status

I'm fixed all tests, the test suite is running without changes in the source code ✌️

react-ace

I've opened securingsincity/react-ace#284, hope it will merged and released soon, so we will can reenable satisfied.

Portal

The test-suite is running without problems with Portal and its deps, so I think that we can impliment createPortal later in separate PRs.

@soederpop
Copy link
Contributor

Great work @layershifter; thanks for getting this done.

@layershifter
Copy link
Member Author

I've updated react-ace and reenable satisfied, LGTM 👍

@levithomason levithomason merged commit bc417d9 into master Oct 10, 2017
@levithomason levithomason deleted the chore/react-16 branch October 10, 2017 06:34
@levithomason
Copy link
Member

Wow, great work @layershifter 🙇

@kud202
Copy link

kud202 commented Oct 10, 2017

@layershifter Thanks for the speedy work on this !

@jeffwillette
Copy link

any idea when this will be released?

@brianespinosa
Copy link
Member

@deltaskelta New releases typically happen over the weekend. @levithomason is a machine.

@levithomason
Copy link
Member

Released in semantic-ui-react@0.75.0

@levithomason
Copy link
Member

Technically, where I am, I missed the weekend by 38 minutes 😄. Also, @layershifter is awesome.

@NickDubelman
Copy link

I'm using 0.75.1 and am still getting "Warning: Stateless function components cannot be given refs. Attempts to access this ref will fail." from all instances of Portal components.

Is this expected?

@layershifter
Copy link
Member Author

Yes, this will be fixed soon.

@NickDubelman
Copy link

I believe my above concern has been resolved in 0.76.0, is that reasonable?

@rijk
Copy link
Member

rijk commented Nov 14, 2017

I believe too. Great work guys! I updated yesterday and it was a 100% smooth ride.

@layershifter
Copy link
Member Author

Yes, Portal supports stateless components as trigger now. It was done in #2219

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

Successfully merging this pull request may close these issues.