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

19 - set selection fix #20

Merged
merged 9 commits into from
Aug 10, 2017
Merged

19 - set selection fix #20

merged 9 commits into from
Aug 10, 2017

Conversation

jwarykowski
Copy link
Contributor

@jwarykowski jwarykowski commented Jun 1, 2017 •
edited by willmcclellan
Loading

Overview

This PR fixes an issue where shouldComponentUpdate doesn't take into consideration any selection change, therefore it doesn't re-render. I've also included some nice little updates.

Updates

  • Updated shouldComponentUpdate
  • Renamed LICENSE and README to consistent naming conventions
  • Updated .npmignore to ignore extra files/folders
  • Added .github templates so we can get some useful information on PR's 💃

Dependencies

N/A

Outstanding Tasks

  • Functional testing using a beta version
  • Update example 🤔

@jwarykowski jwarykowski added the bug label Jun 1, 2017
@jwarykowski jwarykowski mentioned this pull request Jun 1, 2017
Closed
@willmcclellan
Copy link
Member

Nice work @jonathanchrisp. Would be good to update the example so we can verify it and use it as test for future

@chocholand
Copy link

@jonathanchrisp @willmcclellan I added example #24

@jwarykowski
Copy link
Contributor Author

Sorry @chocholand, we've been pretty busy, I'll try and get round to this in the next day or so.

@jwarykowski
Copy link
Contributor Author

@willmcclellan merged the example that @chocholand added in the other PR.

@jwarykowski jwarykowski mentioned this pull request Jun 29, 2017
Closed
@jwarykowski
Copy link
Contributor Author

Hey @chocholand, I've published a new beta version for this package 1.3.4-beta. If possible are you able to test to ensure this version works as expected? We'll test our side also but just want to double check this resolves your original issue before merging.

@chocholand
Copy link

chocholand commented Jul 24, 2017 •
edited
Loading

@jonathanchrisp
your PR(#20) is working. thank you.
But, I reproduced in chrome. #19
you can check if you put "debugger;" at 122 line in Timeline.js.

2017-07-24 6 20 02

@inDream inDream mentioned this pull request Aug 3, 2017
Merged
@willmcclellan
Fix initial options for start/end values
e11333b 
If you use start/end values in conjunction with animation, they won't properly
set when first initialising the components. We're not interested in animating on
first render, so simple fix is to pass options in componentDidMount
@willmcclellan willmcclellan merged commit 463b8f8 into Lighthouse-io:master Aug 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willmcclellan willmcclellan willmcclellan approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jwarykowski @willmcclellan @chocholand