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

Date: Mark getSettings as experimental #10636

Merged
merged 7 commits into from
Oct 25, 2018
Merged

Date: Mark getSettings as experimental #10636

merged 7 commits into from
Oct 25, 2018

Conversation

ajitbohra
Copy link
Member

Fixes #10627

@ajitbohra ajitbohra added [Type] Breaking Change For PRs that introduce a change that will break existing functionality [Package] Date /packages/date labels Oct 15, 2018
@ajitbohra ajitbohra requested review from Soean and aduth October 15, 2018 21:21
Soean
Soean requested changes Oct 15, 2018
View reviewed changes
Copy link
Member

@Soean Soean left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If we deprecate getSettings(), we should change all parts where we import and use it.

@gziolo gziolo added this to the 4.2 - API freeze milestone Oct 16, 2018
@ajitbohra
Copy link
Member Author

ajitbohra commented Oct 17, 2018 •
edited
Loading

@Soean ahh totally missed that deprecating for the first time 😅

@aduth added the hint to discourage the usage. If I am not wrong we need to update the changelog and bump the version for the date package, just noticed changelog the last version is 2.1.0 and package.json version is 2.0.2!

aduth
aduth reviewed Oct 23, 2018
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good, but it missed the 4.1.0 release so we'll need to bump back the deprecated version.

@ajitbohra
Copy link
Member Author

@aduth

version bumped

need help on this

changelog the last version is 2.1.0 and package.json version is 2.0.2!

@aduth
Copy link
Member

aduth commented Oct 24, 2018

@ajitbohra Ah, sorry for missing the question. We never need to touch the package.json in any pull request, and it's expected the version would lag behind if there are "Unreleased" changes. We should include the note in CHANGELOG.md for the deprecation though, as a minor release.

See: https://github.com/WordPress/gutenberg/blob/master/CONTRIBUTING.md#maintaining-changelogs

@ajitbohra
Copy link
Member Author

Note added to change log

Soean
Soean reviewed Oct 24, 2018
View reviewed changes
docs/reference/deprecated.md Outdated Show resolved Hide resolved
aduth
aduth reviewed Oct 24, 2018
View reviewed changes
docs/reference/deprecated.md Outdated Show resolved Hide resolved
@aduth @ajitbohra
Remove deprecated from 4.2
945ce24 
Co-Authored-By: ajitbohra <ajit@lubus.in>
Soean
Soean approved these changes Oct 24, 2018
View reviewed changes
Copy link
Member

@Soean Soean left a comment

Choose a reason for hiding this comment

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

LGTM

Soean
Soean reviewed Oct 25, 2018
View reviewed changes
packages/date/CHANGELOG.md Outdated Show resolved Hide resolved
@Soean @ajitbohra
Marks as unreleased
2e7638e 
Co-Authored-By: ajitbohra <ajit@lubus.in>
aduth
aduth approved these changes Oct 25, 2018
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

In some final testing I came across fact that we export moment as well, which (for different reasons than this) is probably something we don't want to expose on the API. Thoughts?

Not blocking for this pull request.

@aduth aduth merged commit 830c9ec into master Oct 25, 2018
@aduth aduth deleted the fix/10627 branch October 25, 2018 14:51
@youknowriad
Copy link
Contributor

In some final testing I came across fact that we export moment as well, which (for different reasons than this) is probably something we don't want to expose on the API. Thoughts?

We should definitely set "moment" as experimental (or get rid of it) as well. This is something I wanted to do but not sure if I'll have the time to do it.

@ajitbohra
Copy link
Member Author

Yes, I remember the previous discussion around not exposing moment. Will have a look and work on the PR for the same.

antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
@ajitbohra @antpb
Date: Mark getSettings as experimental (WordPress#10636)
667d165 
* Date: Mark getSettings as experimental

* Update all references to __experimentalGetSettings

* Bump deprecated version

* Add change to changelog

* Remove deprecated from 4.2

Co-Authored-By: ajitbohra <ajit@lubus.in>

* Marks as unreleased

Co-Authored-By: ajitbohra <ajit@lubus.in>
daniloercoli added a commit that referenced this pull request Oct 26, 2018
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
7659051 
…rnmobile/merge-blocks-on-backspace

* 'master' of https://github.com/WordPress/gutenberg:
  Do not add isDirty prop to DOM (#11093)
  Format API (#10209)
  Remove 4.2 deprecated features (#10952)
  Update `@wordpress/hooks` README to include namespace mention (#11061)
  Feature: save lock control via actions (#10649)
  Fix usage of `preg_quote()` (#10998)
  Update plugin version to 4.1.1 (#11078)
  Improve preloading request code (#11007)
  Fix dynamic blocks not rendering in the frontend (#11050)
  Media & Text: Fixing vertical alignment of the image (#11025)
  Date: Mark getSettings as experimental (#10636)
  Improve handling of centered 1-col galleries with small images (#11040)
  Use better help text for ALT text input; fixes #8391. (#11052)

# Conflicts:
#	packages/editor/src/components/rich-text/index.native.js
@jeffstieler jeffstieler mentioned this pull request Dec 7, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Soean Soean Soean approved these changes

@aduth aduth aduth approved these changes

Assignees
No one assigned
Labels
[Package] Date /packages/date [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@ajitbohra @aduth @youknowriad @Soean @gziolo