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.

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

Update to React 14 / ReactRouter 1.0 #1981

Merged
merged 15 commits into from
Dec 11, 2015
Merged

Update to React 14 / ReactRouter 1.0 #1981

merged 15 commits into from
Dec 11, 2015

Conversation

aweiksnar
Copy link
Contributor

⚠️ Not ready to merge yet; I'm still finishing up the todos in 1714, and doing lots of QA
⌛ Wanted to open this one up early since it touches almost every file and is a big diff

parrish
parrish reviewed Dec 7, 2015
View reviewed changes
app/collections/preview.cjsx
{@props.collection.display_name}
</Link>
{' '}by{' '}
{if @state.owner
<Link className="user-profile-link" to="user-profile" params={name: @state.owner.login}>
<Link className="/users/#{@state.owner.login}" to="user-profile">
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should be

<Link className="user-profile-link" to="/users/#{@state.owner.login}">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops Yeah absolutely, just updated!

@parrish
Copy link
Contributor

parrish commented Dec 7, 2015

I fixed all (as far as I can tell anyway) of the remaining router changes in #1985.

A few issues I've seen so far.

  • React complains a lot about a div inside a p somewhere. Can't track that down though
  • Admin pages don't render yet
  • Intermittent setState during render errors, primarily in Talk

Just wanted to double check, but it looks like app/collections/home.cjsx isn't used.

@aweiksnar
Copy link
Contributor Author

Yeah just removed collections/home.cjsx 👍 I'll look into those other warnings too. thanks for taking a look at this

@aweiksnar
Copy link
Contributor Author

Admin page rendering and p > div nesting warnings should be all cleared up now

@parrish
Copy link
Contributor

parrish commented Dec 8, 2015

I've resolved every warning/error I can track down. I've seen a setState on an unmounted component error a few times, but can't figure out how to reproduce it (it's probably some sort of race condition)

@parrish
Copy link
Contributor

parrish commented Dec 8, 2015

(Fixed one more)

@aweiksnar aweiksnar force-pushed the react14 branch 2 times, most recently from c9ae448 to 0c7334d Compare December 9, 2015 16:41
@aweiksnar
Copy link
Contributor Author

This should be good to go now, thanks for all the help & testing 🙇 Added back in subdirectory deploys in 893afa1 - let me know if that looks cool to everyone or also any other issues that come up

@parrish
Copy link
Contributor

parrish commented Dec 10, 2015

:shipit: 😎

@brian-c
Copy link
Contributor

brian-c commented Dec 11, 2015

I think all the merge conflicts are stuff from me, I'm fixing 'em now.

@itsravenous itsravenous mentioned this pull request Dec 11, 2015
Closed
brian-c added a commit that referenced this pull request Dec 11, 2015
@brian-c
Merge pull request #1981 from zooniverse/react14
4c8bd7b 
Update to React 14 / ReactRouter 1.0
@brian-c brian-c merged commit 4c8bd7b into master Dec 11, 2015
@aweiksnar aweiksnar deleted the react14 branch December 11, 2015 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aweiksnar @parrish @brian-c