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

Retrospective improvements #30

Merged
merged 5 commits into from Jan 11, 2019
Merged

Retrospective improvements #30

merged 5 commits into from Jan 11, 2019

Conversation

JanLenoch
Copy link

Motivation

Minor refactoring after a peer review w/ @Simply007 .

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs has been updated (if applicable)
  • Temporary settings (e.g. project ID used during development and testing) have been reverted to defaults

Simply007
Simply007 reviewed Dec 31, 2018
View reviewed changes
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

Thanks @JanLenoch,

changes look good!

it would be great to have a list of notes from the pair review so I can check them against the changes. Now I have checked only whether the changes are OK.

One question - how about the starter version?

  • Don't we need to increment it in package.json file?

@JanLenoch
Copy link
Author

If you've been thinking about aligning the version number of the starter to the source plugin, I think this is not necessary. But raising the minor version is a good idea as the dependencies have been upgraded. Well spotted @Simply007 !

Simply007
Simply007 suggested changes Dec 31, 2018
View reviewed changes
package.json Outdated
@@ -4,11 +4,11 @@
"version": "1.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Then I suggest:

Suggested change
"version": "1.0.0",
"version": "1.0.1",

Copy link
Author

Choose a reason for hiding this comment

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

I'll raise that to 1.1.0 (the same way the dependencies' versions jumped), together with the reference being set to gatsby-source-kentico-cloud set to 2.1.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@Simply007
Copy link
Contributor

OK - suggestion created for raising the package version.

Fixes #31, #32
06e1117 
As an immediate solution, the `package-lock.json` file was removed. It should be fine to generate a clean one and commit it later. If the problem occurs again, we can think of further solutions (Yarn).
@Simply007
Copy link
Contributor

see:
#31 (comment)

@Simply007 Simply007 self-assigned this Jan 11, 2019
Simply007
Simply007 approved these changes Jan 11, 2019
View reviewed changes
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

OK, let's leave the #31 opened.

@Simply007 Simply007 merged commit 6ec5804 into master Jan 11, 2019
@Simply007 Simply007 deleted the retrospective-improvements branch January 11, 2019 13:11
@Simply007 Simply007 mentioned this pull request Jan 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Simply007 Simply007 Simply007 approved these changes

Assignees

@Simply007 Simply007

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JanLenoch @Simply007 @petrsvihlik