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

Allow current version id to be false #969

Merged
merged 5 commits into from
Aug 1, 2019

Conversation

mirefly
Copy link
Collaborator

@mirefly mirefly commented Jul 23, 2019

Fixes #955 @kumar303

@codecov-io
Copy link

codecov-io commented Jul 23, 2019

Codecov Report

Merging #969 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #969      +/-   ##
==========================================
+ Coverage    98.8%   98.81%   +<.01%     
==========================================
  Files          55       55              
  Lines        1761     1772      +11     
  Branches      449      454       +5     
==========================================
+ Hits         1740     1751      +11     
  Misses         15       15              
  Partials        6        6
Impacted Files Coverage Δ
src/components/Navbar/index.tsx 100% <ø> (ø) ⬆️
src/reducers/versions.tsx 99.33% <100%> (ø) ⬆️
src/pages/Compare/index.tsx 100% <100%> (ø) ⬆️
src/pages/Index/index.tsx 96.29% <100%> (+0.29%) ⬆️
src/pages/Browse/index.tsx 100% <100%> (ø) ⬆️

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 2df69e9...d780846. Read the comment docs.

@kumar303 kumar303 self-requested a review July 23, 2019 22:03
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Thanks for jumping into this one. It's looking really good but I noticed this issue:

The title disappears from the browse page. If you'd like any tips on where to look for solving it, just let me know.

@kumar303 kumar303 self-assigned this Jul 23, 2019
@mirefly mirefly force-pushed the add-unsetCurrentVersionId-I955 branch from 9e7b293 to 5b757e1 Compare July 24, 2019 16:32
@mirefly
Copy link
Collaborator Author

mirefly commented Jul 24, 2019

@kumar303 I updated Browse and Compare pages. It should be fine now.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Nice work, @mirefly. It's getting close, I just have a few change requests.

src/pages/Browse/index.tsx Outdated Show resolved Hide resolved
src/pages/Compare/index.tsx Outdated Show resolved Hide resolved
src/pages/Browse/index.tsx Outdated Show resolved Hide resolved
src/pages/Compare/index.tsx Outdated Show resolved Hide resolved
@kumar303 kumar303 self-requested a review July 26, 2019 15:54
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

This is looking good, it just needs some additional test cases. If you get stuck setting up the state for those tests, let me know.

Here's a good way to develop it: temporarily set the if statement back to !currentVersionId and watch your new test fail. Fixing the if statement should pass the test.

src/pages/Browse/index.tsx Show resolved Hide resolved
src/pages/Compare/index.tsx Show resolved Hide resolved
@mirefly
Copy link
Collaborator Author

mirefly commented Jul 30, 2019

Thanks @kumar303 I added two test cases.

@kumar303
Copy link
Contributor

@mirefly whoops, it looks like something landed on master that gave you a conflict. Can you merge this branch with master (or rebase on master) and resolve the conflict?

@mirefly mirefly force-pushed the add-unsetCurrentVersionId-I955 branch from e9222a8 to 2ccc112 Compare July 31, 2019 15:55
@mirefly
Copy link
Collaborator Author

mirefly commented Jul 31, 2019

Now it works.

@kumar303 kumar303 self-requested a review August 1, 2019 18:07
Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

Nice work 👍 Thanks!

@kumar303 kumar303 merged commit 7245811 into mozilla:master Aug 1, 2019
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.

Allow the current version to be "unset"
3 participants