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

Update to modern-normalize@0.5.0 #5

Merged
merged 1 commit into from
Dec 1, 2018
Merged

Update to modern-normalize@0.5.0 #5

merged 1 commit into from
Dec 1, 2018

Conversation

azdanov
Copy link
Contributor

@azdanov azdanov commented Nov 24, 2018

Resolves #4

Due to breaking changes I've had to bump version to 1.0.0 since css is no longer exported.

Maybe add css only export as a named export to allow composition for stylesheets? But I think it's not needed.

I've updated all dependencies, linted and fixed files, updated readme and changelog.

Are any changes necessary?

This commit updates to the latest upstream version
`modern-normalize@0.5.0` (1) with the folloing changes:

- Removed moot correction of `h1` font size and margin
  -> sindresorhus/modern-normalize#23
  -> sindresorhus/modern-normalize@ddfe74ac
- Prevent adjustments of font size after orientation changes in iOS
  -> sindresorhus/modern-normalize#28
  -> sindresorhus/modern-normalize@30f4acd4f
- Remove `details` normalization, update `summary` requirement
  -> sindresorhus/modern-normalize#21
  -> sindresorhus/modern-normalize@2dc54f8e

References:
  (1) https://github.com/sindresorhus/modern-normalize/releases/tag/v0.5.0

Co-authored-by: Arctic Ice Studio <development@arcticicestudio.com>
GH-4
Copy link
Owner

@arcticicestudio arcticicestudio 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 helping the project with your contribution 👍

The changes for modern-normalize@0.5.0 looking good, but there are some changes that are not related to #4. Mixing many changes into one PR will reduce the transparency of changes due to missing issue references.

The update to styled-components v4 should be made in a separate PR since it is not required by modern-normalize. To not loose your already altered code I'll extract your related commits for this change into a new PR. Also it is not the goal of this package to force users to use v4 and with that the new component-based styles. It'll break all users that rely on composing the provided CSS with other styles making it impossible to override specific parts of it. We want to only export the styles as interpolated strings, users can then simply create a ModernNormalize component on their own or use the v3 injectGlobal function if they don't want or can't update to v4 yet.

Bumping the version to 1.0.0 would mean it is in production state which is not the case since the upstream project it provides is in development state too (0.5.0 < 1.0.0). Making breaking changes it totally fine when in major version zero 0.x.x:

  1. Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

One commit also includes a bug fix for the documentation (URL of the badge in the changelog) so I'll extract that commit too into a separate issue.

The actual changes for the upstream update are fine, I'll force push this PR, extract the mentioned commits, merge it and keep you updated for the new PR and bug fix issue.

Thanks again for your help 🚀

@arcticicestudio arcticicestudio self-assigned this Dec 1, 2018
@arcticicestudio arcticicestudio added this to the 0.2.0 milestone Dec 1, 2018
@arcticicestudio arcticicestudio changed the title Update styled-modern-normalize Update to modern-normalize@0.5.0 Dec 1, 2018
@arcticicestudio arcticicestudio merged commit 08a4896 into arcticicestudio:develop Dec 1, 2018
@arcticicestudio arcticicestudio removed their assignment Dec 1, 2018
@azdanov
Copy link
Contributor Author

azdanov commented Dec 1, 2018

Thanks for explaining how to do a better PR! Will definitely do more granular PR's in the future and be careful with version numbers (didn't know about 0.x.y).

@azdanov azdanov deleted the bump-deps branch December 1, 2018 15:10
@arcticicestudio
Copy link
Owner

arcticicestudio commented Dec 1, 2018

Your commits are totally fine (saw the prefixes with the scope like docs and the target often in other projects), but it is really easier to split changes up into separate issues (because why not, doesn't cost any cent 😄 ).

I've already created #6 and resolved + merged it in #7 so the update to styled-components v4 is done.
Maybe just a last tip: If you're submitting PRs don't update the version unless explicitly requested since most package maintainers use a specific update workflow when releasing a new package which include automated scripts that bump the version, e.g. I'll now write the changelog and then run my script that'll update all necessary parts of the package and then push it to the NPM registry. A great example are the official React release scripts which I used as inspiration for my own ones 😉

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.

Update to latest version
2 participants